feat(savings): add savings goals#1569
Open
gariasf wants to merge 33 commits intowe-promise:mainfrom
Open
Conversation
- savings_goals: family-scoped goals with target amount, currency, optional target date, color/icon, notes, and a state column (active/paused/completed/archived, defaulting to active). - savings_contributions: per-goal money movements with a source column (initial/manual/auto) and an optional budget_id link used for auto-funded contributions. A partial unique index on (savings_goal_id, budget_id) restricted to auto-source rows guarantees at most one auto-funded contribution per (goal, budget) pair, while leaving manual/initial contributions free. First step of the savings goals feature; picks up #833.
- SavingsGoal: aasm-driven state machine (active/paused/completed/ archived), monetized target, computed current_balance / remaining / progress / monthly_target derived from contributions and target_date. - SavingsContribution: source enum (initial/manual/auto) with custom validation requiring a budget when source=auto, plus monetized amount. - Family: has_many :savings_goals, has_many :savings_contributions through :savings_goals. - Fixtures and unit tests covering validations, lifecycle transitions, balance math, source rules, and family associations.
…_for - Budget#monthly_surplus returns (actual_income - actual_spending) clamped to zero. Sits next to available_to_allocate so it stays in public scope rather than drifting under the private section. - Family::SavingsSummary value object (Data.define) bundles surplus, allocated, available, active_goals, currency for a given budget, with money helpers per field and a fully_allocated? predicate. - Family#savings_summary_for(budget) memoizes one summary per budget id on the family instance, so views and the upcoming AutoFundJob can each request it without recomputing.
- SavingsGoals::AutoFundJob: per-family worker that walks active goals, caps each contribution by min(monthly_target, remaining_pool, remaining_to_target), creates auto-source contributions tied to a budget, and uses an advisory xact lock keyed off the family id so concurrent runs serialize cleanly. The partial unique index is the durable safety net; the lock just produces saner error behaviour. ActiveRecord::RecordNotUnique is caught and treated as a no-op for race tolerance. - SavingsGoals::ScheduleAutoFundsJob: cron entry on the :scheduled queue. Runs at 00:10 UTC on the 1st of each month and enqueues AutoFundJob for every family with at least one active goal and a budget covering the just-closed month. - config/schedule.yml entry registers the cron. - Tests cover surplus=0 no-op, idempotency, monthly-target cap, remaining-amount cap when nearly funded, paused/completed/archived exclusion, manual-contribution coexistence, multi-goal pool exhaustion, and the cron's per-family enqueue + skip-when-no-budget paths.
Switches the goal model from a free-floating virtual envelope (PR #833 / YNAB style) to Firefly III's piggy-bank pattern: each goal is attached to one asset account that holds the actual cash. Progress is now grounded in a real balance, which answers @sokie's verification instinct without extra plumbing and matches the mental model in Sure Discussion #53. - Migration adds non-null `account_id` UUID FK on savings_goals (no production data on this table yet, so a non-null column is safe). - SavingsGoal: belongs_to :account, validates the account belongs to the goal's family and is asset-classified, syncs currency from the account in a before_validation hook so goal.currency always tracks account.currency. Closes the multi-currency open question for v1 — the goal inherits its account's currency. - SavingsContribution: same currency-sync trick; contributions inherit goal.account.currency at validation time, so callers (including AutoFundJob) can stop passing currency explicitly. - Account: has_many :savings_goals, dependent: :destroy so account teardown (e.g. FamilyResetJob, account deletion) cascades cleanly through goals and their contributions. - Fixtures and tests updated to provide the depository account when building goals; new SavingsGoalAccountLinkTest covers the family-match, asset-only, and currency-sync rules. Multi-account-per-goal stays a follow-up.
Three controllers wire the model layer to HTTP, all family-scoped via
`Current.family.savings_goals` / `.budgets` / `.accounts` lookups so
authorization happens implicitly through Active Record (Sure does not
use Pundit; matching that convention).
- SavingsGoalsController: full CRUD + lifecycle member actions
(pause/resume/complete/archive/unarchive). Foreign account_ids and
budget_ids are scoped through Current.family before assignment, so a
hostile form value can never associate a foreign record. Initial
contribution param is consumed in the create flow rather than mass-
assigned to the model. AASM::InvalidTransition is rescued and shown
as a flash alert instead of a 500.
- SavingsContributionsController: new/create/destroy nested under a
goal. Wraps create in `Family.transaction { pg_advisory_xact_lock(...) }`
so concurrent manual contributions on the same family serialize. Drops
any budget_id whose budget does not belong to Current.family
(@sokie's review point on PR #833). source is forced to "manual";
the auto path stays in the job.
- Budgets::SavingsController: budget sub-tab `show` (renders the memoized
SavingsSummary) and `auto_fund` POST that enqueues the existing
AutoFundJob with `(family_id, budget_id)`.
- Routes: nests `resources :savings_goals` (with the lifecycle members
and a `:contributions` sub-resource) at top level, plus a scoped
`budgets/:budget_month_year/savings(/auto_fund)` pair under the
existing budgets resource.
- Placeholder views render enough to make the controllers usable in
development; the real UI (stepper, progress rings, ViewComponents,
Lookbook entries) lands in step 6 along with i18n.
- Controller tests cover the CRUD path, foreign-family rejections (404),
the initial-contribution flow, the lifecycle member actions, the
manual-contribution flow with foreign-budget filtering, and the
cron-style `auto_fund` enqueue assertion.
…-tab
Replaces the placeholder views from the previous commit with the
shippable UI surface for savings goals. All strings are hardcoded
English per CLAUDE.md guidance ("Ignore i18n methods and files. Hardcode
strings in English for now to optimize speed of development").
ViewComponents:
- Savings::ProgressRingComponent: minimal SVG ring, configurable size /
stroke / accent colour, with role="img" + aria-label for screen
readers. No heavy black stroke and no Stimulus dependency, addressing
@nuno-am's review point that the PR #833 ring was too dominant.
- Savings::GoalCardComponent: card layout used by both the goals index
and the budget sub-tab. Wraps the ring with name, account / target
date subtitle, balance / target line, and an aasm state pill.
- Component unit tests for both.
Pages:
- savings_goals#index: state filter (active / paused / completed /
archived / all) rendered as plain link tabs (DS::Tabs is JS-only and
can't drive URL state), responsive grid of GoalCardComponent, empty
state CTA.
- savings_goals#show: bigger ring, goal header with the
edit / pause / resume / complete / archive / unarchive / delete
action icons (gated by aasm `may_…?` predicates), summary trio
(current balance / target / monthly target), notes section, and a
full contributions history with delete buttons.
- savings_goals#new: rendered inside the existing `wizard` layout for
visual continuity with budget setup; shared form partial offers an
optional initial-contribution field, matching PR #833's intent.
- savings_goals#edit: same partial, no initial-contribution field.
- savings_goals/_form: account picker scoped to family asset accounts,
date / money / colour / notes fields. Centred colour picker per
@nuno-am's feedback.
- savings_contributions#new: amount / date / optional budget / notes,
with the budget select scoped to the family's recent budgets.
- budgets/savings#show: surplus / allocated / available trio at the
top, "Auto-fund this month" button, "View all" link to the index,
and the goal cards.
Navigation:
- _budget_nav adds a 3rd "Savings" step alongside the existing
Setup / Categories steps so the sub-tab is reachable from any
budget page. Matches @jjmata's discoverability ask without
promoting savings to a top-level nav item.
Controllers:
- Wired the `wizard` layout for SavingsGoalsController#new.
- Replaced t() flash messages with hardcoded English to match repo
convention; the i18n pass for these (and per-locale YAML scaffolding
reused from PR #833) can land alongside the rest of the locale
strings if maintainers ask for it.
Tests still 0 failures / 7 baseline Plaid errors / 9 skips. New
component tests: 4 added. Total now 971 runs.
UX
- ProgressRingComponent: stroke now uses a Tailwind class for the
background ring (`stroke-current text-gray-200`) and an inline
`style="stroke: <hex>"` for the progress arc. The previous
`var(--color-tertiary)` SVG attribute was silently dropped because
Sure does not define that token, so the ring rendered blank and only
the percent text showed up — exactly what the user reported. Hex
fallbacks: gray-400 < 25%, blue-500 in progress, emerald-500 near /
at target. Per-goal `color:` still wins.
- Goal show page: every action icon (pencil / pause / resume / check /
archive / rotate-ccw / trash) now renders via DS::Button(variant:
:icon) instead of a raw button_to wrapping a bare svg. They share the
same `w-9 h-9 hover:bg-gray-100` shell, so they no longer look like
decoration. Trash button overrides the icon colour to destructive via
Tailwind's `[&_svg]:!text-destructive`. Header now uses the same
state pill as the index card.
- Budget sub-tab: same DS::Button treatment for "Auto-fund this month"
(primary variant, zap icon, sm size) and adds explicit "View all" +
"New goal" buttons next to it so the populated state has the same
CTAs as the empty state.
- Confirms upgraded to `CustomConfirm` (Sure's native confirm-dialog
bridge for Turbo) for both deletion and the auto-fund button.
- Form: `Current.family.accounts` filter now restricts to liquid asset
types (`Depository`, `Investment`, `OtherAsset`). Property and
Vehicle weren't useful piggy-bank backings.
Demo seed
- `Demo::Generator#generate_savings_goals!` adds four goals against the
demo family's depository accounts, exercising the full state machine:
- Awesome vacations (active, ~30% funded, 8 months out, mixed
initial + manual contributions)
- Emergency fund (active, ~20% funded, 12 months out)
- Trip to Japan (active, 0% funded — empty contributions list)
- Paid-off car (completed, fully funded)
Hooked into both `generate_default_data!` and
`generate_new_user_data_for!` so `rake demo_data:default` and the
per-family seed both populate goals.
Suite still 3304 runs / 0 failures / 0 errors / 24 skips.
Earlier we hid the savings entry behind step 3 of the wizard-step nav (Setup → Categories → Savings). The wizard-step visual reads as "complete these in order to finish setting up your budget", which is wrong for savings — savings is a parallel ongoing concern, not a setup task. Users opening /budgets/:month_year had no visible affordance to add a goal. Two changes: - _budget_nav drops the Savings step. The nav goes back to its original Setup → Categories sequence. Savings keeps its dedicated sub-tab page but is no longer mis-classified as setup work. - _savings_summary_card renders on /budgets/:month_year between the donut/actuals row and the categories breakdown. It's a single full-width card matching the categories card style, with: - "New goal" primary CTA + "Manage" secondary link in the header - surplus / allocated / available stat trio when goals exist - the first 3 active GoalCardComponents - a "View all N goals" footer when there are more than 3 - a friendly empty-state nudge when there are none yet - BudgetsController#show now sets @savings_summary via the existing family.savings_summary_for(@Budget) memoized struct. No new query cost beyond what the sub-tab already incurred. The dedicated sub-tab page (/budgets/:month_year/savings) is still where the auto-fund button lives, reachable now via the "Manage" link on the new card instead of the dropped step. Suite still 3304 runs / 0 failures / 0 errors / 24 skips.
User-reported bugs - "New goal" on the budget savings sub-tab and "Add contribution" on the goal show page silently did nothing. Cause: both used DS::Button with an `href:` for plain GET navigation. DS::Button#container wraps in `button_to` which defaults to POST, so we were posting to GET-only /new endpoints and Turbo swallowed the 404. Fix: use DS::Link for GET navigation, keep DS::Button only for POST/PATCH/DELETE actions. IA simplification - Drop the budget savings sub-tab show page (/budgets/:budget_month_year/savings). It duplicated the new summary card on the budget show page and only earned its keep on one button (auto-fund). The sub-tab's other contents (stats trio, active goals) are now on the summary card; the index at /savings_goals is the canonical "all goals" surface. Two surfaces with clear distinct roles instead of three overlapping ones. - Move "Auto-fund this month" onto the summary card, gated to render only when the family has active goals and a positive monthly surplus (otherwise it's a no-op anyway). Confirms via CustomConfirm. - Repoint "Manage" on the summary card from the dropped sub-tab to /savings_goals (the index). - Routes: drop `get "savings"` (the show), keep `post "savings/auto_fund"`. Single-action routes are idiomatic in Sure (cf. `post :copy_previous` on budgets, `delete :reset` on users). Breadcrumbs - SavingsGoalsController and SavingsContributionsController now set @breadcrumbs via Sure's Breadcrumbable concern (the existing pattern the rest of the app uses), so the global breadcrumbs strip in the application layout shows `Home > Savings goals > <goal name> [> Edit]` and gives users a consistent way to navigate back without introducing a custom "← Back" affordance that would lie about its source. Suite still 0 failures / 0 errors. Total drops by 1 (3303) because we removed the sub-tab `show renders summary` test alongside the action.
The field exposed `SavingsContribution#budget_id` to manual contributors, labelled "Apply to budget month (optional)". User flagged the label as opaque — it tells you what to do, not why. Audit confirms the field has zero functional effect on the manual path: - Family savings_summary computes `allocated` as `sum(active_goal.monthly_target_amount)`, NOT as a sum of contributions tagged to a budget. - The partial unique index `(savings_goal_id, budget_id) WHERE source = 'auto'` only dedups auto contributions; manual rows aren't constrained by it. - Nothing in views or reports reads `manual_contribution.budget_id`. - `contributed_at` already answers "when was this saved", which is the question users actually ask. Field came along originally because PR #833 had it, and @sokie's review made me defensive about scoping the lookup. The defence is no longer needed because the form can no longer submit a budget_id at all. Changes: - View `savings_contributions/new.html.erb`: drop the `collection_select`. - Controller `SavingsContributionsController`: * `contribution_params` no longer permits `:budget_id` * removed `lookup_budget` helper and the `@contribution.budget = …` line - Test: removed "create rejects a budget belonging to another family" (no longer reachable from the form). Replaced the regular create test with a positive assertion that manual contributions persist with `budget_id` nil. The auto-fund path is unaffected — `SavingsGoals::AutoFundJob` sets `budget_id` server-side from the family-scoped budget; the partial unique index, the `budget_required_for_auto_source` model validation, and all auto-fund tests continue to exercise it. Schema unchanged — `savings_contributions.budget_id` stays nullable because auto contributions still need it. Suite: 3302 runs / 0 failures / 0 errors / 24 skips. (One less than the previous baseline because we removed the foreign-budget test.)
Tooltips / labels - Every icon-only button on the goal show page (edit pencil, pause / resume / mark-complete / archive / unarchive lifecycle, delete trash, per-contribution delete) now passes both `title:` (HTML hover tooltip) and `aria-label:` (screen-reader name) through to the underlying tag. DS::Buttonish forwards arbitrary opts to button_to / link_to / content_tag, so the attributes land naturally without needing a wrapper. - DS::Tooltip exists in the design system but is built around its own icon (it renders a help-circle with a popover), not as a way to add tooltips to existing components. `title:` is the right primitive here and matches what other Sure templates do for action icons. Index tabs - "All" moves to the leading position and becomes the default tab when no `state` query param is given. Empty-state copy adjusted so it reads "No savings goals yet." under "All" instead of the awkward "No all savings goals." - Controller test renamed and now also asserts the completed fixture shows up under the default filter. Suite still 0 failures / 0 errors.
User flagged dividers rendering as black. Audit revealed those + a few
other places where the new views had drifted off Sure's design tokens
into raw Tailwind palette / undefined names.
The dividers
- `--color-tertiary` is not defined in `app/assets/tailwind/maybe-design-system.css`,
so `border-tertiary` and `divide-tertiary` fell back to the browser
default (black). Swapped to the tokens Sure actually uses for the
same visual role:
- `<hr class="border-tertiary">` in the goal form → `<hr class="my-2 border-secondary">`
(matches `app/views/investments/_value_tooltip.html.erb`,
`app/views/budgets/_budget_nav.html.erb`, etc.)
- Contribution list `divide-y divide-tertiary` → `divide-y divide-alpha-black-200 theme-dark:divide-alpha-white-200`
(matches `app/views/import/cleans/show.html.erb` for similarly
structured row lists)
Form validation alerts
- Hard-coded `bg-red-50 text-red-700 border-red-200` → tokenised
`p-3 rounded-lg border border-destructive bg-red-tint-10 text-destructive`.
`--color-red-tint-10` is `red-500 / 10%` alpha, defined in the design
system. Pattern matches `app/views/splits/new.html.erb` for the same
"soft destructive container" use.
Progress ring
- Stroke colours moved off hex literals (`#9CA3AF`, `#3B82F6`, `#10B981`)
to CSS vars: `var(--color-gray-400)` for "barely started",
`var(--color-blue-500)` for "in progress", and `var(--color-success)`
(the design-token alias for green-600 / green-500-on-dark) for
"near or at target". Same approach as `app/views/shared/_progress_circle.html.erb`,
so progress rings across the app now respond to the same theme
tokens.
- Background ring `text-gray-200` → `text-gray-300` to match Sure's
shared progress circle exactly.
Index tabs
- The active-tab pill `bg-white text-primary shadow-sm` was missing
the dark-mode counterpart. Added `theme-dark:bg-gray-700` to mirror
what `DS::Tabs::VARIANTS[:default][:active_btn_classes]` ships.
Suite still 0 failures / 0 errors. Visual: dividers go from harsh
black to Sure's standard light-grey weight, error alerts pick up the
soft red-tinted destructive treatment used in transaction splits, and
progress rings re-theme correctly under dark mode (and in any future
token tweak).
Audit against Sure conventions revealed our forms were on the wrong
pattern. Categories, tags, family_merchants, settings dialogs, etc.
all wrap `new` and `edit` views in `DS::Dialog` and link to them with
`frame: :modal` so Turbo loads the response into the global modal
frame. The wizard layout I used for `new_savings_goal` is reserved
for sequential setup flows (Setup → Categories on Budget editing),
not for resource creation. This commit aligns.
- savings_goals/new.html.erb, savings_goals/edit.html.erb, and
savings_contributions/new.html.erb now wrap the form in
`DS::Dialog.new { with_header(title:) + with_body { ... } }` —
literal copy of the categories/tags/family_merchants pattern.
- SavingsGoalsController#new drops `render layout: "wizard"`;
SavingsGoalsController#create / #update and
SavingsContributionsController#create now `respond_to do |format|`
with `format.turbo_stream { render turbo_stream: turbo_stream.action(:redirect, ...) }`
so a modal submit closes the dialog and navigates the parent frame
via Turbo (matches the redirect_target_url pattern Categories uses).
HTML format still 302-redirects so direct URL submits and the
controller tests keep working unchanged.
- Every link that opens a creation/edit form gets `frame: :modal`:
- savings_goals#index "New goal" CTA (top-right + empty state)
- savings_goals#show edit pencil + "Add contribution" button
- budgets/_savings_summary_card "New goal" button
Goal show page stays a full route (it's a permanent destination, not
a form). Lifecycle action icons (pause / resume / complete / archive
/ unarchive / delete) stay as in-place PATCH/DELETE buttons — they're
direct actions, not modal flows.
Suite still 0 failures / 0 errors. Existing controller tests hit the
HTML path so they cover the non-modal flow; the modal flow rides on
the same underlying create/update logic with only the response format
branch added.
Four changes from the audit, in priority order:
1. Color palette → Category::COLORS
Goals shared the same generic Tailwind 400-shade palette I'd hard-coded
(#F87171 / #FB923C / …). Categories — the closest existing concept —
ship a richer custom palette in `app/models/category.rb`:
`%w[#e99537 #4da568 #6471eb #db5a54 #df4e92 #c44fe9 #eb5429 #61c9ea #805dee #6ad28a]`.
Goal forms now iterate `Category::COLORS` directly, so picking a goal
colour produces something that visually meshes with the user's
categories instead of the generic web-blue / mint-green palette.
Demo seed and savings_goals fixtures updated to use values from this
palette too.
2. Color picker UX → details disclosure with avatar preview
Replaced the always-on row of nine swatches with a Sure-style
`<details>` reveal: a 12×12 avatar circle (background 12% color-mix,
foreground hex, "target" lucide icon) acts as the summary, click
expands to the palette. A tiny new Stimulus controller
`savings_goal_color_controller.js` wires the avatar to live-update on
radio change. Mirrors the visual interaction in
`app/javascript/controllers/category_controller.js` without dragging
in the icon-picker / Pickr / hex-picker machinery (savings goals are
colour-only).
3. Show-page actions → DS::Menu kebab + visible Edit
The previous header crammed pencil + pause + resume + check + archive
+ rotate-ccw + trash inline. None of them are common enough to earn
that real estate; the visible primary is Edit. Lifecycle and Delete
collapse into a `DS::Menu(variant: :icon)` kebab using `with_item`,
filtered by AASM `may_…?` predicates so only valid transitions
surface. Delete carries `destructive: true` and the existing
CustomConfirm. Pattern matches user_menu / categories index kebab.
4. Bigger ring with balance text inside, drop the stats trio
The 96×96 ring + separate three-stat card was redundant — current
balance / target / monthly target each appeared twice. Replaced with
a single header card: a 180×180 ring rendering three stacked tspans
inside (current balance · "of $target" · "X%"), beside the goal
identity (name, state pill, account, target date) and the kebab.
Monthly target moves to a single line under the identity. Donut-
style information density matching `app/views/budgets/_budget_donut`.
ProgressRingComponent now accepts `label_lines:` (defaults to
["#{percent}%"] so existing GoalCardComponent + budget summary card
keep their compact percent display unchanged).
Suite still 3302 / 0 failures / 0 errors / 24 skips.
Tests added (15 new assertions, 3302 → 3317):
Models
- SavingsContributionTest:
* "syncs currency from goal.account on save" — guards the
`before_validation :sync_currency_from_goal` callback added in the
account-linkage commit.
* "syncs currency on subsequent updates" — verifies the sync survives
re-validation.
Controllers
- SavingsGoalsControllerTest:
* "update silently drops a foreign account_id" — symmetry with the
existing create-side test. Confirms a foreign-family account_id
cannot be re-pointed via update.
* "update with invalid params re-renders edit" — covers the
unprocessable_entity path.
* "lifecycle action with invalid transition flashes alert and
redirects" — exercises the rescue AASM::InvalidTransition path
(archived -> pause), which previously had no test.
Components
- ProgressRingLabelLinesTest:
* default percent label renders one tspan
* custom `label_lines: [...]` renders stacked tspans (the goal show
page's "$balance / of $target / X%" stack)
* `color:` override produces the inline stroke style on the arc
- GoalCardComponentStatePillTest:
* pill classes for each AASM state (active / paused / completed /
archived)
* subtitle falls back to account name when target_date is absent
FamilySavingsSummaryTest:
- "summary handles a family with no active goals" — empty active_goals,
zero allocated, available == surplus
- "memoization is keyed by budget id, not shared across budgets" —
proves the @savings_summary_cache hash actually keys by budget.id
(a regression that hashed everything to one entry would have passed
the previous tests)
Security / Brakeman
- `pg_advisory_xact_lock(#{key})` in AutoFundJob and
SavingsContributionsController switched to
`sanitize_sql_array(["SELECT pg_advisory_xact_lock(?)", key])` so
Brakeman's static analysis no longer flags raw interpolation. The
actual injection risk was nil (the key is a derived integer), but
the maintainer pre-PR checklist runs Brakeman.
- SavingsGoalsController dropped `:account_id` from the strong-params
permit list. The controller already manually scoped lookups via
`lookup_account` against `Current.family.accounts`; permitting the
attribute as well let Brakeman's PermitAttributes check warn on
potential mass-assignment of a foreign-key field. Manual assignment
via `params.dig(:savings_goal, :account_id)` is now the single path
through which an account_id can land on the model. No behaviour
change — existing controller tests (foreign-account create, foreign-
account update) still pass.
Brakeman re-run on the savings files: "No warnings found". Suite:
3317 / 0 failures / 0 errors / 24 skips.
User flagged the multi-line numbers in the goal show ring as visually broken. Cause: I was rendering them as `<text>` + multiple `<tspan>`s inside the SVG with mixed font-sizes via Tailwind classes, plus baseline maths driven by ems. SVG text styling resolution varies and the dy shifts assumed a single font size for the whole stack, so the lines ended up off-centre and uneven. Sure's existing `app/views/budgets/_budget_donut.html.erb` doesn't put text inside the SVG either — it positions HTML divs absolutely over the chart container. Refactored ProgressRingComponent to the same pattern: - Outer wrapper is a `position: relative` div sized to the ring. - SVG sits at `inset-0`, drawing only the two circles. No `<text>` / `<tspan>` inside. - Text overlay is a flex-column-centred container with one `<span>` per label line. First line uses `text-primary font-medium` at a size derived from `primary_size_class` (text-xs <80px → text-lg >160px), subsequent lines are `text-secondary text-xs`. `leading-tight` keeps the stack compact. - Single-line use cases (goal cards' "21%") still render via the same overlay — visually identical to before, just no longer routed through SVG text. Component tests updated to assert `<span>` instead of `<tspan>`, and a new test pins the `primary_size_class` mapping at four ring sizes to catch regressions in the scale function. Suite: 3317 → 3318 (one new test). 0 failures / 0 errors / 24 skips.
… tests
Two real bugs the audit surfaced, plus edge tests around the rest.
A. Multi-currency goals were summed wrong in family.savings_summary_for
`allocated = active.sum { |g| g.monthly_target_amount || 0 }`
summed BigDecimals across goals regardless of currency. A family
with a USD goal at $500/mo and an EUR goal at €300/mo got an
`allocated` of 800 (raw, currency-blind) and AutoFundJob would have
debited a USD surplus to fund a EUR goal. Demo accounts span USD,
EUR, and GBP, so this was reachable.
Fix: filter active_goals by `currency: budget.currency` inside
`Family#savings_summary_for`. Other-currency goals are still
active; they just don't enter this budget's surplus math, and the
AutoFundJob (which reads summary.active_goals) inherits the filter
for free. Per-budget memoization still works because the cache key
is budget.id.
B. Currency-swap-via-account-swap silently corrupted contributions
`before_validation :sync_currency_from_account` overwrites
`goal.currency` from the new account. Existing
`SavingsContribution` rows kept their old currency, and
`current_balance = sum(:amount)` then mixed currencies. Edit form
could trigger this (e.g. switching backing from USD checking to
EUR savings).
Added `currency_locked_once_contributions_exist` validation: once
a goal has any contribution, the account currency must match the
contributions' currency. Account swap to same-currency accounts
stays free. Account swap before any contributions exist also stays
free.
C. Misc:
- SavingsGoal#name now `length: { maximum: 255 }`. A 50k-char name
would have torn the goal card layout.
- Brakeman re-run still clean.
Edge tests added (most catch the above; some pin behaviour we'd want
to keep stable):
- name length cap
- progress_percent caps at 100 when over-funded
- remaining_amount clamps at zero when over-funded
- monthly_target_amount returns full remaining when target_date is in
the past (months_remaining == 0)
- currency-locked validation rejects account swap to a different
currency once contributions exist
- currency-swap allowed when there are no contributions yet
- cascading destroy when the backing account is deleted (covers the
Account.has_many :savings_goals, dependent: :destroy added in the
account-linkage commit)
- savings_summary_for excludes goals in other currencies
- create with initial_contribution >= target keeps state active and
progress_percent = 100% (verifies the over-funded-on-create path
doesn't silently flip the goal to completed)
Suite: 3318 → 3327. 0 failures / 0 errors / 24 skips.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Savings Goals feature: new models/migrations (goals, contributions), family integrations and budget surplus logic, controllers/routes, background jobs (scheduler + auto-fund), UI components and views, Stimulus color controller, demo/fixtures, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Controller as "Budgets::SavingsController"
participant JobQueue as "ActiveJob"
participant AutoFund as "SavingsGoals::AutoFundJob"
participant DB as "Database"
participant Goals as "SavingsGoal"
participant Contributions as "SavingsContribution"
Browser->>Controller: POST /budgets/:month/savings/auto_fund
Controller->>DB: resolve budget for family
Controller->>JobQueue: enqueue AutoFundJob(family_id, budget_id)
Controller-->>Browser: redirect to budget show (notice)
Note over AutoFund,DB: async processing
AutoFund->>DB: acquire advisory lock (family scope)
AutoFund->>DB: load budget, compute monthly_surplus
AutoFund->>DB: preload existing auto contributions for budget
AutoFund->>Goals: fetch active goals (ordered)
loop per active goal while pool > 0
AutoFund->>Goals: skip if already auto-funded or ineligible
alt eligible
AutoFund->>AutoFund: compute amount (cap by monthly_target, remaining, pool)
AutoFund->>Contributions: create auto contribution (nested tx)
Contributions->>DB: insert row (handle unique constraint)
AutoFund->>AutoFund: decrement pool
end
end
AutoFund->>DB: release advisory lock
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
gariasf
changed the title
brin-security-scanner
Bot
added
pr:verified
![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: 37d4f9a2e7
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (10)
app/components/savings/progress_ring_component.html.erb (1)
9-12: Replace the hard-coded gray token.
text-gray-300bypasses the repo’s functional token palette; please switch the base ring to the matching semantic token so the component stays consistent with the rest of the UI. As per coding guidelines, use functional design system tokens instead of raw Tailwind color values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/savings/progress_ring_component.html.erb` around lines 9 - 12, The base SVG circle uses a hard-coded Tailwind color class "text-gray-300" (in the tag.circle element); replace that class with the repository's semantic/functional color token used for neutral or muted UI elements (the same token other components use for base rings, e.g., the design-system class like "text-muted" or the project's equivalent semantic token), updating the class attribute on tag.circle so the component uses the functional design-system token instead of a raw color.db/migrate/20260427144444_add_account_to_savings_goals.rb (1)
1-5: Consider includingaccount_idin the initialCreateSavingsGoalsmigration.The
account_idcolumn withnull: falseis being added in a separate migration immediately after table creation. Since both migrations run in the same batch and the table is newly created, there's no data integrity risk. However, the design can be simplified: ifaccount_idis mandatory (asnull: falseindicates), it should be included in theCreateSavingsGoalsmigration rather than added separately. This makes the table structure clearer and avoids multi-step setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260427144444_add_account_to_savings_goals.rb` around lines 1 - 5, The new migration AddAccountToSavingsGoals adds a non-null UUID foreign key :account to savings_goals; instead move that column into the original CreateSavingsGoals migration so the table is created with account_id from the start. Update the CreateSavingsGoals migration to include add_reference :account, type: :uuid, null: false, foreign_key: true (or the equivalent column/foreign key definition) and remove the separate AddAccountToSavingsGoals class; ensure the foreign key and null constraint are present in the CreateSavingsGoals migration to keep schema creation atomic and clear.test/jobs/savings_goals/auto_fund_job_test.rb (2)
69-78: Test title overstates covered states.This test currently verifies only
paused, but the title claimspaused / completed / archived. Please either add the missing two states or narrow the title.Suggested update
- test "skips paused / completed / archived goals" do + test "skips paused, completed, and archived goals" do paused = `@family.savings_goals.create`!( account: accounts(:depository), name: "Paused", target_amount: 1000, state: "paused", target_date: 3.months.from_now.to_date ) + completed = `@family.savings_goals.create`!( + account: accounts(:depository), + name: "Completed", target_amount: 1000, + state: "completed", target_date: 3.months.from_now.to_date + ) + archived = `@family.savings_goals.create`!( + account: accounts(:depository), + name: "Archived", target_amount: 1000, + state: "archived", target_date: 3.months.from_now.to_date + ) Budget.any_instance.stubs(:monthly_surplus).returns(5_000) SavingsGoals::AutoFundJob.new.perform(`@family.id`, `@budget.id`) assert_equal 0, paused.savings_contributions.auto.count + assert_equal 0, completed.savings_contributions.auto.count + assert_equal 0, archived.savings_contributions.auto.count end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/jobs/savings_goals/auto_fund_job_test.rb` around lines 69 - 78, The test title "skips paused / completed / archived goals" claims three states but only creates a goal in state "paused"; update the test to either change the title to only mention "paused" or create additional goal fixtures for the "completed" and "archived" states and assert they also have zero auto contributions after calling SavingsGoals::AutoFundJob.new.perform(`@family.id`, `@budget.id`); locate the test by the string "skips paused / completed / archived goals" and the variable paused and modify the test to include goals with state "completed" and "archived" (or rename the title) and corresponding assertions for savings_contributions.auto.count.
90-100: “Fairly” is not asserted by this test.The test only proves the funded total does not exceed surplus. Consider renaming it to match current assertions, or add distribution assertions to validate fairness semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/jobs/savings_goals/auto_fund_job_test.rb` around lines 90 - 100, The test named "competes goals fairly when surplus is limited and stops once exhausted" only asserts the total funded amount (SavingsContribution.auto.where(budget: `@budget`).sum(:amount)) is <= monthly surplus (Budget#monthly_surplus stubbed to 150); either rename the test to reflect this (e.g., "does not exceed monthly surplus") or add explicit fairness assertions after running SavingsGoals::AutoFundJob.new.perform(`@family.id`, `@budget.id`) — for example, fetch individual contributions for the created goals (via SavingsContribution.auto.where(budget: `@budget`, savings_goal: ...)) and assert their distribution matches the expected fairness policy (equal shares or proportional to target_amount/remaining_amount as implemented), while keeping the existing total-funded assertion.app/models/demo/generator.rb (1)
1285-1289: Minor duplication in depository-account lookup.Same relation is built twice; extracting once improves readability and avoids repeated query composition.
Suggested cleanup
- asset_account = family.accounts.where(classification: "asset", accountable_type: "Depository").alphabetically.first + depository_accounts = family.accounts.where(classification: "asset", accountable_type: "Depository").alphabetically + asset_account = depository_accounts.first return unless asset_account - checking = family.accounts.where(classification: "asset", accountable_type: "Depository").alphabetically.second || asset_account + checking = depository_accounts.second || asset_account🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/demo/generator.rb` around lines 1285 - 1289, Extract the repeated relation into a single variable to avoid rebuilding the same query: assign something like depository_accounts = family.accounts.where(classification: "asset", accountable_type: "Depository").alphabetically, then set asset_account = depository_accounts.first and checking = depository_accounts.second || asset_account; update usages of asset_account/checking accordingly (references: asset_account, checking, and the repeated family.accounts.where(...).alphabetically relation).test/controllers/savings_contributions_controller_test.rb (1)
28-33: Strengthendestroytest with response assertion.The test verifies deletion but not the HTTP outcome. Add an
assert_redirected_to savings_goal_path(@goal)(or expected fallback) to lock the controller contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/controllers/savings_contributions_controller_test.rb` around lines 28 - 33, In the "destroy removes the contribution" test, after calling delete via savings_goal_contribution_path(`@goal`, contribution) and the assert_difference, add an assertion that the controller responds with the expected redirect (e.g. assert_redirected_to savings_goal_path(`@goal`)) to verify the HTTP outcome; update the test "destroy removes the contribution" to include this assert_redirected_to referencing `@goal` so the controller contract is enforced.app/views/savings_goals/_form.html.erb (2)
16-19: Move account-loading query out of the ERB template.The ActiveRecord query on Line 17 puts domain/data-access logic in the view. Prepare these options in controller/component code and pass them in as locals.
As per coding guidelines: "Keep domain logic out of views: compute values like ... data transformations in the component file, not the template file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/savings_goals/_form.html.erb` around lines 16 - 19, The ERB partial _form.html.erb currently runs an ActiveRecord query inline; move that query into controller/component code (e.g., in the SavingsGoalsController actions that render the form or in the form's view component) and expose the result as a local or instance variable like backing_accounts; specifically, compute backing_accounts = Current.family.accounts.where(classification: "asset", accountable_type: %w[Depository Investment OtherAsset]).alphabetically in the controller/component (for new/edit/create/update render paths) and then update the partial to call collection_select :account_id, backing_accounts, :id, :name with the same options and prompt.
41-41: Replace raw Tailwind color class with a design-system token.
peer-checked:ring-gray-500on Line 41 bypasses the functional token system and can drift from theme semantics.As per coding guidelines: "Use functional tokens from design system ... instead of raw Tailwind classes ... never hardcoded color values."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/savings_goals/_form.html.erb` at line 41, The span element that currently uses the raw Tailwind color class peer-checked:ring-gray-500 should be changed to use the design-system functional token instead; locate the span with class starting "block w-8 h-8 rounded-full ring-2 ..." and replace peer-checked:ring-gray-500 with the corresponding design-system ring token (e.g., peer-checked:ring-[design-token-name] or the provided utility like peer-checked:ring-token-<semantic-name>) so the checked ring color follows the theme tokens rather than a hardcoded gray value.app/jobs/savings_goals/auto_fund_job.rb (1)
48-81: Avoid per-goalexists?queries in the funding loop.Line 50 triggers one query per goal via
already_auto_funded?, which can turn into an N+1 pattern for large families.Query-efficient approach
def run(family, budget) summary = family.savings_summary_for(budget) pool = summary.surplus.to_d return if pool <= 0 + auto_funded_goal_ids = family.savings_contributions + .where(budget_id: budget.id, source: "auto") + .pluck(:savings_goal_id) + .to_h { |id| [ id, true ] } summary.active_goals.each do |goal| break if pool <= 0 - next if already_auto_funded?(goal, budget) + next if auto_funded_goal_ids[goal.id] @@ SavingsContribution.create!( @@ ) + auto_funded_goal_ids[goal.id] = true pool -= amount end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/savings_goals/auto_fund_job.rb` around lines 48 - 81, The per-goal exists? call in already_auto_funded? causes an N+1 query inside the summary.active_goals loop; prefetch the set of already auto-funded goal IDs for the given budget once (e.g., query SavingsContribution.where(budget_id: budget.id, source: "auto", savings_goal_id: summary.active_goals.map(&:id)).pluck(:savings_goal_id) and memoize to a Set) and replace the per-goal exists? check with a fast in-memory lookup (e.g., set.include?(goal.id)) in the loop or update already_auto_funded? to consult that memoized set instead of hitting the DB. Ensure the lookup uses budget.id and source "auto" and covers savings_goal_id to match the original semantics.test/models/savings_goal_test.rb (1)
168-168: Avoid broadrescuein test data setup.On Line 168, rescuing everything around
families(:empty_family)can hide unrelated failures. Prefer explicit fixture usage or explicit record creation without blanket rescue.Suggested fix
- other_family = families(:empty_family) rescue Family.create!(name: "Other", locale: "en", date_format: "%Y-%m-%d", currency: "USD") + other_family = Family.create!(name: "Other", locale: "en", date_format: "%Y-%m-%d", currency: "USD")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/savings_goal_test.rb` at line 168, Replace the broad rescue around families(:empty_family) with an explicit existence check or explicit creation: attempt to fetch the fixture/record using families(:empty_family) or Family.find_by(...) and if not present call Family.create! with the attributes; remove the blanket rescue so unrelated errors surface, and ensure the test uses either the fixture name or a clear Family.create! call (refer to families(:empty_family) and Family.create!) to locate and update the setup.
🤖 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/savings/goal_card_component.html.erb`:
- Around line 8-15: Extract presentation logic from the template into the
Savings::GoalCardComponent by adding two methods: state_label (which returns a
localized string, e.g. t("savings.goals.states.#{goal.state}")) and
target_amount_money (which returns Money.new(goal.target_amount,
goal.currency)); then update the template to call state_label instead of
goal.state.titleize and to call target_amount_money inside helpers.format_money
instead of constructing Money in the view, and add the corresponding locale keys
under savings.goals.states for each goal state.
In `@app/controllers/savings_goals_controller.rb`:
- Around line 40-43: The current assignment in the SavingsGoalsController uses
submitted_account_id and unconditionally sets `@savings_goal.account` =
lookup_account(submitted_account_id), which sets the account to nil when
lookup_account fails and can block unrelated attribute updates; change the logic
so you only assign `@savings_goal.account` if submitted_account_id is present and
lookup_account(submitted_account_id) returns a non-nil Account (i.e., call
lookup_account, check the result, and only set `@savings_goal.account` when the
result is truthy), otherwise leave `@savings_goal.account` unchanged (or handle
the invalid account case explicitly without nulling the association).
- Around line 24-26: The creation of `@savings_goal` and the subsequent
handle_initial_contribution call must be wrapped in a single database
transaction so both succeed or both rollback; replace the separate
`@savings_goal.save` plus later create! with an ActiveRecord::Base.transaction
block that creates/saves the SavingsGoal (use save! or create!) and then calls
handle_initial_contribution inside the same transaction so any exception (from
create!) rolls back the goal; update the controller flow to rescue
ActiveRecord::RecordInvalid (or let the transaction raise) and set flash/render
appropriately on failure, and apply the same transactional fix to the other
goal-creation flow that also calls handle_initial_contribution.
In `@app/javascript/controllers/savings_goal_color_controller.js`:
- Around line 9-13: The connect() method currently imperatively attaches
"change" listeners to each radio (this.colorTargets.forEach(...)) which can
duplicate on reconnect; remove that eventListener loop from connect() and
instead add declarative data-action="change->savings-goal-color#updateAvatar" to
the radio inputs in the ERB template. Keep the controller's updateAvatar method
but adjust its signature to accept the Stimulus event (e.g., event) or read the
target value via event.target.value so the declarative binding works; ensure
colorTargets and updateAvatar remain present and only perform DOM updates (no
manual event wiring) in the controller.
In `@app/jobs/savings_goals/schedule_auto_funds_job.rb`:
- Around line 9-15: The budget lookup uses an exact match on target_month_start
(variable target_month_start and line using family.budgets.find_by(start_date:
target_month_start)), which skips families with custom month boundaries; change
the lookup to find the family's budget that covers the target date instead of
matching start_date exactly (e.g. query family.budgets for a record where
start_date <= target_month_start and end_date >= target_month_start / or
end_date is null), so the loop in Family.joins(:savings_goals)...find_each
correctly finds the budget that spans the target month.
In `@app/models/family/savings_summary.rb`:
- Around line 21-23: The current fully_allocated? method incorrectly requires
allocated.positive? so a budget with zero surplus but no allocations returns
false; change fully_allocated? (in class SavingsSummary) to only check
available.to_d.zero? (remove the allocated.to_d.positive? requirement) so a
zero-surplus budget is considered fully allocated; keep the to_d conversion to
handle numeric/string inputs consistently.
In `@app/models/savings_contribution.rb`:
- Line 11: The amount validation currently uses numericality: true which allows
0 and negatives; update the SavingsContribution model's amount validation
(validates :amount, presence: true, numericality: true) to enforce a positive
value by replacing numericality: true with a numericality option requiring
greater_than: 0 (e.g., numericality: { greater_than: 0 }) so zero and negative
contributions are rejected during validation.
In `@app/models/savings_goal.rb`:
- Around line 108-110: Replace the single-item check of
savings_contributions.first.currency with a full-collection check: gather the
distinct non-blank currencies from savings_contributions (e.g.
savings_contributions.where.not(currency: nil).distinct.pluck(:currency)) and
then if that set is empty or exactly matches account.currency allow the change,
otherwise call errors.add(:account, "cannot be changed to a different currency
once the goal has contributions"); update references to existing_currency to use
this distinct currencies collection in the validation.
In `@app/views/budgets/_savings_summary_card.html.erb`:
- Around line 5-77: Replace every hardcoded user-facing string in this partial
(e.g. the "Savings goals" heading, DS::Link instances' text like "Manage", "New
goal", the CustomConfirm title/body, the metric labels "Monthly
surplus"/"Allocated to goals"/"Available", the "Auto-fund this month" button,
the "View all X goals" link, and the "No active savings goals." block) with i18n
lookups using t('<scope>.<key>') and pass any dynamic values (like
summary.active_goals.size or formatted money) as interpolation variables; update
the calls to CustomConfirm.new(title: ..., body: ...) and DS::Link.new(text:
...) to use t(...) and add the corresponding keys to config/locales/en.yml
(including pluralization/interpolation for the "View all %{count} goals" label
and any button/tooltips), ensuring all visible strings are covered and
tests/locales are updated accordingly.
In `@app/views/savings_contributions/new.html.erb`:
- Around line 1-24: Replace all hard-coded user-facing strings in the modal with
i18n lookups: change the dialog header call (dialog.with_header) to use
t("savings_contributions.new.title", goal_name: `@savings_goal.name`) and replace
labels and button text provided to f.money_field, f.date_field, f.text_area, and
f.submit with t("savings_contributions.new.amount"),
t("savings_contributions.new.date"), t("savings_contributions.new.notes"), and
t("savings_contributions.new.submit") respectively; then add corresponding keys
to config/locales/en.yml (e.g. savings_contributions: new: title: "Add
contribution to %{goal_name}" amount: "Amount" date: "Date" notes: "Notes
(optional)" submit: "Add contribution") so the view uses t() everywhere and the
title interpolates goal_name.
In `@app/views/savings_goals/_form.html.erb`:
- Around line 38-43: The color radio buttons generated from Category::COLORS
(see the f.radio_button :color usage and savings_goal_color_target) currently
only show a swatch and lack accessible names; update each radio option so screen
readers get a clear label by adding an explicit accessible name (for example add
an aria-label on the f.radio_button or include a visually-hidden span (class
"sr-only") inside the surrounding label with the color name or hex) while
keeping the visible swatch UI and existing classes/targets intact.
- Around line 14-63: Replace all hardcoded user-visible strings in this partial
with i18n lookups using the t() helper and corresponding locale keys; update the
labels/placeholders/prompts in f.text_field :name, f.collection_select
:account_id (prompt and label), f.money_field :target_amount, f.date_field
:target_date, the "Color" summary text and the avatar placeholder (icon alt/text
if any), f.text_area :notes, the "Initial contribution" label and
number_field_tag placeholder, and the f.submit text so they call
t('savings_goal.form.*') keys (or similarly namespaced keys), and add matching
entries to config/locales/en.yml. Ensure values like the prompt "Choose an asset
account", placeholders "e.g. Trip to Japan" and "What is this goal for?", and
button texts use t(), and preserve existing interpolation/HTML where needed.
In `@app/views/savings_goals/edit.html.erb`:
- Around line 2-4: The dialog title is hardcoded in edit.html.erb
(dialog.with_header(title: "Edit savings goal")); change it to use the i18n
helper t(...) with a new locale key (e.g. savings_goals.edit.title) and add that
key and value to config/locales/en.yml under the appropriate namespace; ensure
the view calls t('savings_goals.edit.title') (or chosen key) instead of the raw
string so the title is localized.
In `@app/views/savings_goals/index.html.erb`:
- Around line 3-4: The view contains hard-coded English strings and raw Tailwind
color classes; replace all visible copy in
app/views/savings_goals/index.html.erb (the <h1> title, description <p>, tab
labels, CTA and empty-state messages referenced in the diff) with t() calls
using new keys (e.g. savings_goals.index.title, .description, .tabs.* , .cta,
.empty_state.*) and add those keys to config/locales/en.yml, and swap raw color
classes like bg-white and theme-dark:bg-gray-700 for the design system
functional tokens (use the project’s surface/background/text token classes used
elsewhere, e.g. bg-surface or text-primary-token) so no user-facing strings or
raw color values remain in the template.
In `@app/views/savings_goals/new.html.erb`:
- Around line 2-4: The dialog title is hardcoded in dialog.with_header(title:
"New savings goal"); change it to use the i18n helper (e.g. title:
t("savings_goals.new.title")) so the string is localized, and add the
corresponding key and English text to config/locales/en.yml
(savings_goals.new.title: "New savings goal"); keep the same callsite
(dialog.with_header) and ensure tests or views reference the new locale key if
needed.
In `@app/views/savings_goals/show.html.erb`:
- Around line 29-37: Replace all hardcoded user-facing strings in
savings_goals/show.html.erb (e.g., "Notes", "Contributions", "Add contribution",
action labels, empty-state text) with t(...) i18n lookups and move copy into the
locale yml; update any state display like `@savings_goal.state.titleize` to use a
translated label (e.g., t("savings_goals.states.#{`@savings_goal.state`}"));
replace direct date formatting `@savings_goal.target_date.strftime`("%b %Y") with
locale-aware formatting via l(`@savings_goal.target_date`, format: :month_year) or
I18n.l and add the :month_year format to locale dates; do the same for other
occurrences called out in the comment (lines ~39-43, 50-90, 97-98, 104-107,
118-120, 130-137) so all UI copy and server-formatted dates are localized.
In `@db/schema.rb`:
- Around line 1225-1235: Add a DB-level check so rows with source='auto' cannot
have NULL budget_id to uphold the uniqueness enforced by index
index_auto_contributions_unique_per_goal_per_budget on table
savings_contributions; create a migration (e.g.,
AddAutoContributionBudgetConstraint) that adds a check constraint named
chk_savings_contributions_auto_requires_budget with condition "(source <>
'auto') OR (budget_id IS NOT NULL)" and run migrations to update schema.rb so
budget_id nullability for auto contributions is prevented at the DB level.
---
Nitpick comments:
In `@app/components/savings/progress_ring_component.html.erb`:
- Around line 9-12: The base SVG circle uses a hard-coded Tailwind color class
"text-gray-300" (in the tag.circle element); replace that class with the
repository's semantic/functional color token used for neutral or muted UI
elements (the same token other components use for base rings, e.g., the
design-system class like "text-muted" or the project's equivalent semantic
token), updating the class attribute on tag.circle so the component uses the
functional design-system token instead of a raw color.
In `@app/jobs/savings_goals/auto_fund_job.rb`:
- Around line 48-81: The per-goal exists? call in already_auto_funded? causes an
N+1 query inside the summary.active_goals loop; prefetch the set of already
auto-funded goal IDs for the given budget once (e.g., query
SavingsContribution.where(budget_id: budget.id, source: "auto", savings_goal_id:
summary.active_goals.map(&:id)).pluck(:savings_goal_id) and memoize to a Set)
and replace the per-goal exists? check with a fast in-memory lookup (e.g.,
set.include?(goal.id)) in the loop or update already_auto_funded? to consult
that memoized set instead of hitting the DB. Ensure the lookup uses budget.id
and source "auto" and covers savings_goal_id to match the original semantics.
In `@app/models/demo/generator.rb`:
- Around line 1285-1289: Extract the repeated relation into a single variable to
avoid rebuilding the same query: assign something like depository_accounts =
family.accounts.where(classification: "asset", accountable_type:
"Depository").alphabetically, then set asset_account = depository_accounts.first
and checking = depository_accounts.second || asset_account; update usages of
asset_account/checking accordingly (references: asset_account, checking, and the
repeated family.accounts.where(...).alphabetically relation).
In `@app/views/savings_goals/_form.html.erb`:
- Around line 16-19: The ERB partial _form.html.erb currently runs an
ActiveRecord query inline; move that query into controller/component code (e.g.,
in the SavingsGoalsController actions that render the form or in the form's view
component) and expose the result as a local or instance variable like
backing_accounts; specifically, compute backing_accounts =
Current.family.accounts.where(classification: "asset", accountable_type:
%w[Depository Investment OtherAsset]).alphabetically in the controller/component
(for new/edit/create/update render paths) and then update the partial to call
collection_select :account_id, backing_accounts, :id, :name with the same
options and prompt.
- Line 41: The span element that currently uses the raw Tailwind color class
peer-checked:ring-gray-500 should be changed to use the design-system functional
token instead; locate the span with class starting "block w-8 h-8 rounded-full
ring-2 ..." and replace peer-checked:ring-gray-500 with the corresponding
design-system ring token (e.g., peer-checked:ring-[design-token-name] or the
provided utility like peer-checked:ring-token-<semantic-name>) so the checked
ring color follows the theme tokens rather than a hardcoded gray value.
In `@db/migrate/20260427144444_add_account_to_savings_goals.rb`:
- Around line 1-5: The new migration AddAccountToSavingsGoals adds a non-null
UUID foreign key :account to savings_goals; instead move that column into the
original CreateSavingsGoals migration so the table is created with account_id
from the start. Update the CreateSavingsGoals migration to include add_reference
:account, type: :uuid, null: false, foreign_key: true (or the equivalent
column/foreign key definition) and remove the separate AddAccountToSavingsGoals
class; ensure the foreign key and null constraint are present in the
CreateSavingsGoals migration to keep schema creation atomic and clear.
In `@test/controllers/savings_contributions_controller_test.rb`:
- Around line 28-33: In the "destroy removes the contribution" test, after
calling delete via savings_goal_contribution_path(`@goal`, contribution) and the
assert_difference, add an assertion that the controller responds with the
expected redirect (e.g. assert_redirected_to savings_goal_path(`@goal`)) to verify
the HTTP outcome; update the test "destroy removes the contribution" to include
this assert_redirected_to referencing `@goal` so the controller contract is
enforced.
In `@test/jobs/savings_goals/auto_fund_job_test.rb`:
- Around line 69-78: The test title "skips paused / completed / archived goals"
claims three states but only creates a goal in state "paused"; update the test
to either change the title to only mention "paused" or create additional goal
fixtures for the "completed" and "archived" states and assert they also have
zero auto contributions after calling
SavingsGoals::AutoFundJob.new.perform(`@family.id`, `@budget.id`); locate the test
by the string "skips paused / completed / archived goals" and the variable
paused and modify the test to include goals with state "completed" and
"archived" (or rename the title) and corresponding assertions for
savings_contributions.auto.count.
- Around line 90-100: The test named "competes goals fairly when surplus is
limited and stops once exhausted" only asserts the total funded amount
(SavingsContribution.auto.where(budget: `@budget`).sum(:amount)) is <= monthly
surplus (Budget#monthly_surplus stubbed to 150); either rename the test to
reflect this (e.g., "does not exceed monthly surplus") or add explicit fairness
assertions after running SavingsGoals::AutoFundJob.new.perform(`@family.id`,
`@budget.id`) — for example, fetch individual contributions for the created goals
(via SavingsContribution.auto.where(budget: `@budget`, savings_goal: ...)) and
assert their distribution matches the expected fairness policy (equal shares or
proportional to target_amount/remaining_amount as implemented), while keeping
the existing total-funded assertion.
In `@test/models/savings_goal_test.rb`:
- Line 168: Replace the broad rescue around families(:empty_family) with an
explicit existence check or explicit creation: attempt to fetch the
fixture/record using families(:empty_family) or Family.find_by(...) and if not
present call Family.create! with the attributes; remove the blanket rescue so
unrelated errors surface, and ensure the test uses either the fixture name or a
clear Family.create! call (refer to families(:empty_family) and Family.create!)
to locate and update the setup.
🪄 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: c5fa631d-349d-46b0-aaaf-5be1d374f64c
📒 Files selected for processing (46)
app/components/savings/goal_card_component.html.erbapp/components/savings/goal_card_component.rbapp/components/savings/progress_ring_component.html.erbapp/components/savings/progress_ring_component.rbapp/controllers/budgets/savings_controller.rbapp/controllers/budgets_controller.rbapp/controllers/savings_contributions_controller.rbapp/controllers/savings_goals_controller.rbapp/javascript/controllers/savings_goal_color_controller.jsapp/jobs/savings_goals/auto_fund_job.rbapp/jobs/savings_goals/schedule_auto_funds_job.rbapp/models/account.rbapp/models/budget.rbapp/models/demo/generator.rbapp/models/family.rbapp/models/family/savings_summary.rbapp/models/savings_contribution.rbapp/models/savings_goal.rbapp/views/budgets/_savings_summary_card.html.erbapp/views/budgets/show.html.erbapp/views/savings_contributions/new.html.erbapp/views/savings_goals/_form.html.erbapp/views/savings_goals/edit.html.erbapp/views/savings_goals/index.html.erbapp/views/savings_goals/new.html.erbapp/views/savings_goals/show.html.erbconfig/routes.rbconfig/schedule.ymldb/migrate/20260427141452_create_savings_goals.rbdb/migrate/20260427141453_create_savings_contributions.rbdb/migrate/20260427144444_add_account_to_savings_goals.rbdb/schema.rbtest/components/savings/goal_card_component_test.rbtest/components/savings/progress_ring_component_test.rbtest/controllers/budgets/savings_controller_test.rbtest/controllers/savings_contributions_controller_test.rbtest/controllers/savings_goals_controller_test.rbtest/fixtures/savings_contributions.ymltest/fixtures/savings_goals.ymltest/jobs/savings_goals/auto_fund_job_test.rbtest/jobs/savings_goals/schedule_auto_funds_job_test.rbtest/models/budget_monthly_surplus_test.rbtest/models/family_savings_summary_test.rbtest/models/family_savings_test.rbtest/models/savings_contribution_test.rbtest/models/savings_goal_test.rb
Two real bugs flagged by the automated reviewer (both correct), with covering tests. 1. Budgets::SavingsController#set_budget called Budget.param_to_date without `family:`, so for families with a non-default `month_start_day` (e.g. 15) the budget_month_year route param fell through to `beginning_of_month` and find_or_bootstrap resolved to a different period. Auto-fund could enqueue funding against the wrong month. Fix: pass `family: Current.family` to param_to_date and `user: Current.user` to find_or_bootstrap, mirroring upstream BudgetsController#set_budget. 2. SavingsGoals::ScheduleAutoFundsJob#perform hardcoded `Date.current.last_month.beginning_of_month` for every family. Families with a non-default `month_start_day` store budgets on their own boundary, so `find_by(start_date: ...)` returned nil and monthly auto-funding was silently skipped. Fix: compute `target_month_start` per family via `family.custom_month_start_for(Date.current.last_month)`. The helper returns the 1st when month_start_day is 1, so default families are unaffected. Tests added: - Budgets::SavingsControllerTest "auto_fund resolves the right budget for a custom-month-start family" — sets month_start_day = 15, posts to the period-15 budget, asserts the right budget id is enqueued. - SavingsGoals::ScheduleAutoFundsJobTest "uses the family's custom month-start when resolving the previous-period budget" — same setup on the cron path. Suite: 3327 → 3329. 0 failures / 0 errors / 24 skips.
Triaged the automated review against the actual code; fixing the
findings that are real, declining the ones that disagree with project
conventions or upstream precedent.
Real bugs
- SavingsGoalsController#create now wraps the goal save plus
handle_initial_contribution in a single ActiveRecord::Base.transaction
with save!/create! semantics, rescuing ActiveRecord::RecordInvalid
and re-rendering :new. Previously, if the goal saved but the initial
contribution failed, the goal would persist while the request 500'd.
- SavingsGoalsController#update no longer assigns @savings_goal.account
when lookup_account returns nil. The previous unconditional assign
set the association to nil for foreign account_ids, which fails the
belongs_to :account presence requirement and blocked unrelated
attribute changes (e.g. a name edit) in the same request. The test
for foreign-account drops now also asserts the name edit lands.
- SavingsContribution amount validation tightened from `numericality:
true` to `numericality: { greater_than: 0 }` so zero and negative
manual contributions are rejected.
- SavingsGoal#currency_locked_once_contributions_exist switched from a
single `savings_contributions.first.currency` check to a
`where.not(currency: [nil, "", account.currency]).exists?` check, so
defensively-mixed historical currencies don't slip past when the
first row happens to match the new account's currency.
- AutoFundJob#run prefetches the set of already-auto-funded goal ids
for the budget once via a single SELECT, then uses an in-memory
`to_set#include?` instead of an N+1 `goal.savings_contributions
.where(...).exists?` per goal. Removes the standalone
already_auto_funded? helper.
DB-level integrity
- New migration `add_auto_contribution_budget_check_constraint` adds
`(source <> 'auto') OR (budget_id IS NOT NULL)` as a check
constraint on savings_contributions. The partial unique index
`WHERE source = 'auto'` on (savings_goal_id, budget_id) cannot
enforce uniqueness when budget_id is NULL (Postgres treats NULLs as
distinct), so without the check, raw SQL or a future bulk-insert
could bypass the model-level `budget_required_for_auto_source`
validation. Defense-in-depth.
Cleanup / nits
- Family::SavingsSummary#fully_allocated? was leftover dead code and
is removed.
- Savings::GoalCardComponent now exposes `state_label` and
`target_amount_money` so the template stops constructing
Money.new(...) inline and `goal.state.titleize` lives in Ruby.
- Color radios in the goal form get `aria: { label: "Goal color #{hex}" }`
so screen readers can identify each option.
- Stimulus controller switched from imperative addEventListener in
connect() to a declarative
`data-action="change->savings-goal-color#updateAvatar"` binding on
each radio. Avoids double-binding on Turbo reconnect.
- Backing-account query moved out of the form partial into a
before_action `set_backing_accounts` on SavingsGoalsController, which
also covers the create/update re-render paths. Form takes a
`backing_accounts:` local instead of running an AR query inline.
- Demo::Generator#generate_savings_goals! dedups the repeated
`family.accounts.where(...).alphabetically` relation into a single
local.
- savings_goal_test replaces a `families(:empty_family) rescue
Family.create!(...)` with an explicit `Family.create!`. Bare rescues
hide unrelated errors.
- Auto-fund "skips paused / completed / archived goals" test now
actually exercises all three states. Companion test "competes goals
fairly" renamed to "total auto-funded amount never exceeds the
monthly surplus", which is what its assertion actually proves.
- Contribution destroy controller test adds the missing
`assert_redirected_to savings_goal_path(@goal)`.
Declined (with reasoning, in case maintainers want to reverse any)
- All 7 i18n complaints. CLAUDE.md is explicit: "Ignore i18n methods
and files. Hardcode strings in English for now to optimize speed of
development." Following project policy.
- `text-gray-300` in ProgressRingComponent background. Matches
`app/views/shared/_progress_circle.html.erb` exactly; this is
Sure's existing precedent for neutral progress-ring strokes.
- `bg-white theme-dark:bg-gray-700` in the index tabs is a literal
copy of `DS::Tabs::VARIANTS[:default][:active_btn_classes]` from the
design system component itself.
- `peer-checked:ring-gray-500` in the color picker is identical to
what `app/views/categories/_form.html.erb` ships for its swatches.
- The previous-period budget skip in ScheduleAutoFundsJob — codex's
snapshot was on the pre-fix code; the fix landed in commit b5ea5fe
ahead of this review.
- Squashing the AddAccountToSavingsGoals migration into the original
CreateSavingsGoals would require force-pushing over commits that
have active review threads on this PR (codex's own + earlier
comments), orphaning context. Easy to fold at merge time via
squash-merge if maintainers prefer.
Suite: 3329 / 0 failures / 0 errors / 24 skips. Brakeman clean on
touched files. Rubocop / erb_lint / npm lint clean.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/models/savings_goal.rb (1)
50-52: Minor: dead|| 0fallback.
ActiveRecord::Calculations#sumalready returns0(notnil) for empty relations, so the|| 0here never fires. Safe to drop.♻️ Optional cleanup
- def current_balance - savings_contributions.sum(:amount) || 0 - end + def current_balance + savings_contributions.sum(:amount) + end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/savings_goal.rb` around lines 50 - 52, The method current_balance contains a redundant fallback (|| 0) because ActiveRecord's sum already returns 0 for empty relations; remove the dead fallback so current_balance simply returns savings_contributions.sum(:amount) (update the current_balance method accordingly).app/components/savings/goal_card_component.rb (1)
18-25: Consider localizing state and date formatting.
state.titleizeandtarget_date.strftime('%b %Y')produce English-only output. Other parts of the app render dates/labels viaI18n.land translation keys, which would be more consistent for a feature shown on user-facing dashboards.♻️ Optional refactor
def target_summary - return goal.account.name if goal.target_date.nil? - "#{goal.target_date.strftime('%b %Y')} · #{goal.account.name}" + return goal.account.name if goal.target_date.nil? + "#{I18n.l(goal.target_date, format: '%b %Y')} · #{goal.account.name}" end def state_label - goal.state.titleize + I18n.t("savings_goals.states.#{goal.state}", default: goal.state.titleize) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/savings/goal_card_component.rb` around lines 18 - 25, Replace the hardcoded English formatting in target_summary and state_label with I18n calls: use I18n.l(goal.target_date, format: :month_year) (or an app-specific date format) instead of strftime('%b %Y') and use I18n.t with a namespaced key like "savings.goal.states.#{goal.state}" (with a sensible fallback to goal.state.titleize) instead of goal.state.titleize; update target_summary to fall back to goal.account.name when target_date is nil and ensure the translation keys and date format are added to your locale files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/jobs/savings_goals/auto_fund_job.rb`:
- Around line 21-25: The top-level Family.transaction around acquire_lock! and
run is fine but the method-level rescue around SavingsContribution.create! masks
unique-constraint failures and causes the entire Family.transaction to be
aborted and rolled back; to fix this, wrap each call that creates a contribution
(the SavingsContribution.create! call inside the run/goal loop) in its own
savepoint by executing it inside ActiveRecord::Base.transaction(requires_new:
true) (or ActiveRecord::Base.connection.transaction(requires_new: true)) so a
RecordNotUnique on one goal rolls back only that savepoint instead of the outer
Family.transaction; keep the rescue for RecordNotUnique if desired but ensure
the create is inside the requires_new transaction so earlier contributions
persist.
---
Nitpick comments:
In `@app/components/savings/goal_card_component.rb`:
- Around line 18-25: Replace the hardcoded English formatting in target_summary
and state_label with I18n calls: use I18n.l(goal.target_date, format:
:month_year) (or an app-specific date format) instead of strftime('%b %Y') and
use I18n.t with a namespaced key like "savings.goal.states.#{goal.state}" (with
a sensible fallback to goal.state.titleize) instead of goal.state.titleize;
update target_summary to fall back to goal.account.name when target_date is nil
and ensure the translation keys and date format are added to your locale files.
In `@app/models/savings_goal.rb`:
- Around line 50-52: The method current_balance contains a redundant fallback
(|| 0) because ActiveRecord's sum already returns 0 for empty relations; remove
the dead fallback so current_balance simply returns
savings_contributions.sum(:amount) (update the current_balance method
accordingly).
🪄 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: 7803696b-0b0d-49eb-bf9a-70bee459d270
📒 Files selected for processing (18)
app/components/savings/goal_card_component.html.erbapp/components/savings/goal_card_component.rbapp/controllers/savings_goals_controller.rbapp/javascript/controllers/savings_goal_color_controller.jsapp/jobs/savings_goals/auto_fund_job.rbapp/models/demo/generator.rbapp/models/family/savings_summary.rbapp/models/savings_contribution.rbapp/models/savings_goal.rbapp/views/savings_goals/_form.html.erbapp/views/savings_goals/edit.html.erbapp/views/savings_goals/new.html.erbdb/migrate/20260427194701_add_auto_contribution_budget_check_constraint.rbdb/schema.rbtest/controllers/savings_contributions_controller_test.rbtest/controllers/savings_goals_controller_test.rbtest/jobs/savings_goals/auto_fund_job_test.rbtest/models/savings_goal_test.rb
✅ Files skipped from review due to trivial changes (5)
- app/views/savings_goals/edit.html.erb
- app/views/savings_goals/new.html.erb
- db/migrate/20260427194701_add_auto_contribution_budget_check_constraint.rb
- test/jobs/savings_goals/auto_fund_job_test.rb
- app/models/savings_contribution.rb
🚧 Files skipped from review as they are similar to previous changes (5)
- app/components/savings/goal_card_component.html.erb
- app/javascript/controllers/savings_goal_color_controller.js
- test/controllers/savings_contributions_controller_test.rb
- app/views/savings_goals/_form.html.erb
- db/schema.rb
Two findings from the second CodeRabbit review pass. 1. AutoFundJob#run wrapped the whole goal loop in Family.transaction and rescued ActiveRecord::RecordNotUnique at the outer scope. In Postgres, a unique-constraint violation aborts the entire enclosing transaction at the database level — rescuing the Ruby exception doesn't undo that. The eventual COMMIT becomes ROLLBACK, so a single duplicate from a concurrent worker would have wiped out every successful contribution created earlier in the loop. Fix: each `SavingsContribution.create!` now runs inside `ActiveRecord::Base.transaction(requires_new: true)` so a savepoint scopes the rollback to just that iteration. The outer Family.transaction (which holds the advisory lock) stays clean and prior successful contributions persist. Test: stubs the first `SavingsContribution.create!` to raise RecordNotUnique and the second to succeed; without the savepoint, the second call would hit `PG::InFailedSqlTransaction`. 2. SavingsGoal#current_balance had a `|| 0` fallback after `savings_contributions.sum(:amount)`. ActiveRecord::Calculations#sum returns 0 (not nil) for empty relations, so the fallback was dead code. Dropped with a comment. Suite: 3329 → 3330. 0 failures / 0 errors / 24 skips.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/jobs/savings_goals/auto_fund_job.rb (1)
28-30: Optional: preferfind_byoverwhere(...).first.Minor idiomatic nit — matches the style on Line 18 and avoids the implicit
LIMIT 1ordering quirk.♻️ Suggested refactor
def default_budget(family) - family.budgets.where(start_date: Date.current.beginning_of_month).first + family.budgets.find_by(start_date: Date.current.beginning_of_month) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/jobs/savings_goals/auto_fund_job.rb` around lines 28 - 30, Replace the use of where(...).first in the default_budget method with an idiomatic single-record finder: change the default_budget(family) implementation to use find_by(start_date: Date.current.beginning_of_month) instead of family.budgets.where(...).first so it matches the style used elsewhere (e.g., line 18) and avoids the implicit ordering quirk of where(...).first.
🤖 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/jobs/savings_goals/auto_fund_job.rb`:
- Around line 28-30: Replace the use of where(...).first in the default_budget
method with an idiomatic single-record finder: change the default_budget(family)
implementation to use find_by(start_date: Date.current.beginning_of_month)
instead of family.budgets.where(...).first so it matches the style used
elsewhere (e.g., line 18) and avoids the implicit ordering quirk of
where(...).first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61256b8f-9f8f-4e8e-99dd-15416a3d10fe
📒 Files selected for processing (3)
app/jobs/savings_goals/auto_fund_job.rbapp/models/savings_goal.rbtest/jobs/savings_goals/auto_fund_job_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/savings_goal.rb
- test/jobs/savings_goals/auto_fund_job_test.rb
…le goals Two findings from the latest codex pass on 69c8112: 1) savings_contributions.budget_id was created with the default ON DELETE behaviour (NO ACTION). Family#destroy walks budgets before savings_goals, so a family that had ever auto-funded would fail to destroy with a foreign-key violation, breaking InactiveFamilyCleaner and any user-initiated family teardown. Fix: new migration drops and re-adds the FK with `on_delete: :cascade`. Manual / initial contributions don't carry budget_id and are untouched. Auto contributions are tied semantically to a specific budget anyway -- the audit trail of "$X auto-funded for Apr 2026" has no anchor once the Apr 2026 budget is gone. Nullify isn't safe here: the existing CHECK `(source <> 'auto') OR (budget_id IS NOT NULL)` would fire on nullification because the row's source stays 'auto'. Also adds `Budget has_many :savings_contributions` for AR awareness (no `dependent:` -- the DB cascade owns destroy). 2) The "Auto-fund this month" button rendered whenever the family had any active goal and a positive surplus, but AutoFundJob skips goals with `monthly_target_amount.nil?` (= goals without a target_date). A family whose only active goals are undated would see the button, click it, get a success flash and zero contributions. Fix: add `Family::SavingsSummary#fundable_goals` -- active goals with a positive monthly target -- and gate the button on it. The summary card itself still renders for any active goals, just without the misleading auto-fund CTA. Tests: - savings_contribution_test: budget destroy cascades to its auto rows; destroying a fresh family that has auto contributions doesn't raise. - family_savings_summary_test: fundable_goals excludes undated active goals; fundable_goals empty when every active goal is undated. Suite: 3332 -> 3336. 0 failures / 0 errors / 24 skips.
….md domain note Walked the two new forms against the rest of the codebase's form patterns (transactions, valuations, categories, tags, family_merchants) and aligned: - savings_goals/_form: name field gets `autofocus: true` to match the modal-form pattern (categories, tags, family_merchants); account picker gets `variant: :logo` + `text-ellipsis` class to match transactions/_form (logo treatment in the option, name truncation for long account names); target_amount money_field gets `disable_currency: true` matching valuations -- currency syncs from account.currency and is locked once contributions exist, so the picker should never have looked editable. - SavingsGoalsController#new prefills `currency: Current.family.currency` so the disabled money_field shows the family default instead of the USD fallback before an account is selected. - savings_contributions/new: amount money_field gets the same `disable_currency: true` plus `currency_value_override: @savings_goal.currency` since the contribution's own currency is nil until before_validation syncs it; contributed_at gets `max: Date.current` so the date picker won't accept future-dated contributions (matching transactions/_form's "when did this happen" pattern). Default-to-today is already handled by the controller's `contributed_at: Date.current` initializer. Also adds a one-line domain-model bullet to CLAUDE.md noting Family owns Budgets + SavingsGoals, with SavingsContributions sourced initial / manual / auto. Three comparable feature PRs (Budgeting V1, Nested Categories, Rules V1) shipped without README/CHANGELOG touches, so this is the only doc surface that warranted an update. No behavior change beyond the form-state polish; existing controller tests cover the create/update paths and stay green (3336 / 0 / 0 / 24).
…account Two N+1 paths the perf audits flagged on the new feature: 1) Every render that touches a SavingsGoal called `current_balance` (and friends) more than once. `progress_percent` re-derives it, `monthly_target_amount` -> `remaining_amount` -> `current_balance` stacks again, the budget summary card calls all three on every active goal, and the index page calls them on every card. Each call fired its own `SUM(amount)` -- two or three SQL round-trips per goal per render. Memoize on the instance: instances are request-scoped (or one per AutoFundJob iteration), and the contributions for a goal don't change inside a single render pass. Index page query count drops by ~2/3 at typical scale and ~100 SUMs at 50-goal power-user scale. Memoization covers `current_balance`, `current_balance_money`, `remaining_amount`, `remaining_amount_money`, `progress_percent`, `monthly_target_amount`. The latter two use the `defined?` guard instead of `||=` because `progress_percent` is legitimately 0 for fresh goals and `monthly_target_amount` is legitimately nil for undated goals -- `||=` would re-fire on every call in those states. 2) `goal.account` is touched by GoalCardComponent#target_summary and by the goal index template. Neither query path eager-loaded it, so each card triggered its own Account SELECT. Add `.includes(:account)` to SavingsGoalsController#index and to the active-goals load inside `Family#savings_summary_for`. That collapses the N+1 on /savings_goals (50 -> 1 extra query at power-user scale) and the same on /budgets/:m (3 -> 1). Suite: 3336 / 0 / 0 / 24. No behavior change; only fewer queries.
…cron Two operator-scale wins flagged by the perf audits, follow-up to the per-instance memoization in the previous commit: 1) `SavingsGoal.with_current_balance` scope. Pre-aggregates the contribution sum into a virtual `current_balance_total` column via LEFT OUTER JOIN + GROUP BY. `#current_balance` reads from that attribute when present and falls back to the lazy SUM otherwise, so direct callers (tests, console, anything that doesn't go through the scope) keep working unchanged. Threaded through `Family#savings_summary_for` and `SavingsGoalsController#index`. AutoFundJob's loop (`monthly_target_amount`/`remaining_amount` per goal) now reads the prefetched balance through memoization instead of firing one SUM per goal inside the advisory lock. At 50 active goals per family that's 50 in-lock round-trips collapsed into 1. 2) `ScheduleAutoFundsJob` -> bulk enqueue. The old loop enqueued one Sidekiq job per family with one budget SELECT per family. At 10k families on a single cron tick that's 10k Redis pushes plus 10k SELECTs. Now we walk in 1k-family batches, group each batch by its resolved month-start (so families with a non- default `month_start_day` still pick up the right budget), do one `Budget.where(family_id: …, start_date: X)` per distinct start_date inside the batch, and ship the resulting jobs to the queue via `ActiveJob.perform_all_later` (one Redis round- trip per batch instead of one per family). Suite: 3336 / 0 / 0 / 24. No behavior change.
Collaborator
Author
|
@codex review |
Collaborator
Author
|
@jjmata This is now ready for a first high-level human review. At this stage, I'm mostly interested in architecture and UX feedback, to see if we're on the right direction, or if we should discuss a different approach. Thank you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2efa44a73c
ℹ️ 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".
… auto-fund
Codex P1 on the latest pass: SavingsContributionsController hashed the
key as "savings_contribution:<family>" while AutoFundJob hashed as
"savings_auto_fund:<family>", so the two paths landed on different
pg_advisory_xact_lock keys and didn't actually serialize against each
other. Concrete race: a manual contribution and an auto-fund running
concurrently each compute remaining_amount from their own snapshot,
both insert, and the goal ends up overfunded past target.
Fix: extract `SavingsGoal.advisory_lock_key_for(family_id)` (a
deterministic SHA1 -> 63-bit positive int) and call it from both
sites. Same family id now produces the same key everywhere, which is
what the original "this lock keeps manual contributions tidy too"
comment meant to do.
The lock key namespace is new ("savings_goals:family:<id>"), but
since pg_advisory_xact_lock is transaction-scoped there are no
cross-deploy lock-state concerns.
Test: a stability + range check on the helper. The integration that
matters (both code paths calling it) is one-line greppable in each
file, so I didn't add a fragile capture-based regression test.
Suite: 3336 -> 3337. 0 failures / 0 errors / 24 skips.
…ard buttons Two PWA / narrow-viewport bugs visible in the savings index and the budget summary card: 1) The state-filter tabs in savings_goals/index used `flex-1` per pill but no `min-w-0`. Flex items default to `min-width: auto` (= min-content), so a long label (long translation, custom override) ignored flex-1's shrink and pushed its pill past the container while the others got squeezed. Adopt the same idiom `app/views/shared/_transaction_type_tabs.html.erb` already uses: `flex-1 min-w-0` on the link plus `<span class="truncate">` around the label. Equal-width segmented look stays on roomy viewports; long labels gracefully ellipsize on narrow. 2) The Manage / New goal / Auto-fund this month button row in budgets/_savings_summary_card had no `flex-wrap`, so on a narrow PWA viewport the third button clipped past the card edge. Add `flex-wrap` so buttons wrap to a second row instead of clipping -- matches the splits/edit, splits/new pattern in the repo for action button rows. Both fixes are minimal-surgical and follow existing Sure responsive idioms. Suite stays 3337 / 0 / 0 / 24.
github-project-automation
Bot
moved this to Todo
in Latest release train (v0.7.1) - May 2026
jjmata
removed
the
contributor:flagged
Collaborator
|
Let me get Thanks for pick it back up ... 🚀 |
# Conflicts: # db/schema.rb
brin-security-scanner
Bot
added
the
contributor:flagged
jjmata
removed
the
contributor:flagged
brin-security-scanner
Bot
added
the
contributor:flagged
jjmata
removed
the
contributor:flagged
Collaborator
@nuno-am's comment was:
I agree that consistency would be nice here. Let's do it! |
Collaborator
Personally I would take the |
Collaborator
I think funds could come from multiple places, so shipping v1 with this is probably important. |
sure-design
suggested changes
May 6, 2026
sure-design
left a comment
sure-design
left a comment
There was a problem hiding this comment.
Reviewed the current head (8f790af) against all prior review comments. The vast majority of issues were properly addressed — the three hardest concurrency/correctness problems (advisory-lock key mismatch, RecordNotUnique rolling back all prior contributions, wrong month boundaries for custom-start families) are all genuinely fixed and verified in the code.
Three items still need attention before merge:
1. Missing DB-level CHECK constraint for auto contributions (P1 — marked resolved but not implemented)
The CodeRabbit P1 ("DB-level integrity gap for auto contributions") was closed pointing to commit d9e51a1, but no CHECK constraint appears in any migration. The model validator budget_required_for_auto_source blocks source='auto' rows with a NULL budget_id through ActiveRecord, but PostgreSQL treats NULLs as non-equal in unique-index checks — so multiple (savings_goal_id, NULL, source='auto') rows can be inserted by bypassing the application layer, silently defeating the idempotency guarantee.
The fix is one migration:
add_check_constraint :savings_contributions,
"(source <> 'auto') OR (budget_id IS NOT NULL)",
name: "chk_savings_contributions_auto_requires_budget"2. Initial-contribution validation errors are silently swallowed (new issue introduced by the atomicity fix)
SavingsGoalsController#create wraps goal save + initial contribution in a single transaction:
begin
ActiveRecord::Base.transaction do
@savings_goal.save!
handle_initial_contribution(@savings_goal) # can raise RecordInvalid
end
rescue ActiveRecord::RecordInvalid
return render :new, status: :unprocessable_entity
endWhen handle_initial_contribution raises RecordInvalid, the transaction rolls back and :new is re-rendered — but the error lives on a local SavingsContribution object inside handle_initial_contribution, not on @savings_goal. The form re-renders with an empty error block and no visible message. A user who submits an invalid initial-contribution amount (e.g. negative, wrong format) gets a silent failure.
Simplest fix: rescue RecordInvalid inside handle_initial_contribution and proxy the message onto the goal:
def handle_initial_contribution(goal)
amount = params.dig(:savings_goal, :initial_contribution).to_d
return unless amount.positive?
goal.savings_contributions.create!(
amount: amount, source: "initial", contributed_at: Date.current
)
rescue ActiveRecord::RecordInvalid => e
e.record.errors.each { |err| goal.errors.add(:base, err.full_message) }
raise
end3. Raw Tailwind color classes in index.html.erb (marked resolved — still present)
The active-tab branch in the state-filter nav still uses:
"bg-white theme-dark:bg-gray-700 text-primary shadow-sm"bg-white and theme-dark:bg-gray-700 are raw color values. Per the design system rules, functional tokens should be used instead (e.g. bg-container). CodeRabbit flagged this, it was closed, but the current head is unchanged.
Minor / non-blocking
-
Hardcoded
"%b %Y"format inl()calls —show.html.erbandGoalCardComponent#target_summaryboth passformat: "%b %Y"toI18n.l. This is better than rawstrftime(month names will localize) but the format itself is still hardcoded rather than a named locale format likeformat: :month_year. Low-impact, but worth completing since the i18n pass was otherwise thorough. -
on_delete: :cascadevs:nullify— the P1 reviewer suggested:nullify; the implementation chose:cascade, which deletes auto contributions when a budget row is deleted. Since budgets are auto-bootstrapped and not user-deletable this is fine in practice, but worth an explicit acknowledgement from a maintainer that cascade (rather than nullify) is the intended policy.
Everything else from the prior reviews is confirmed fixed. Once items 1–3 above are addressed this looks good to me.
Generated by Claude Code
brin-security-scanner
Bot
added
the
contributor:verified
Collaborator
Author
|
Thanks for the feedback, I will run this through Claude design too, as we discussed and see what it outputs. @jjmata |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A piggy-bank style savings goals feature for the community fork. Each goal links to one of your asset accounts. You fill it from manual contributions or an opt-in monthly auto-fund. It shows up as a summary card on
/budgets/:month_year, next to the surplus that funds it.This picks up the work from #833 (closed for codebase drift) and reimplements from scratch against current
mainrather than rebasing, so the architectural points raised in the previous review could be addressed cleanly. Closes #415 and #1570.Screenshots
Budget show page with the Savings goals card
Goal show page
New goal modal with the color picker expanded
Add contribution modal
Savings goals index
Auto-fund confirmation
Walkthrough
Open
/budgets/:month_year: there's a new full-width "Savings goals" card between the donut/actuals row and the categories breakdown. It shows a surplus / allocated / available trio, the first three active goals, andManage/New goal/Auto-fund this monthbuttons.Click
New goalto get a modal with the form (asset-account picker, target amount, optional target date, color picker that uses theCategory::COLORSpalette, optional initial contribution). On submit you land on the goal show page: a 180×180 progress ring with current / target / percent overlaid, identity (state pill, account, target date, monthly target) beside it, an Edit pencil, and a kebab menu for lifecycle transitions (pause / resume / mark-complete / archive / restore / delete)./savings_goalsis the full index with state-filter tabs. Clicking auto-fund on the budget card enqueues aSavingsGoals::AutoFundJobfor the current month; the same job runs as a Sidekiq cron at10 0 1 * *(config/schedule.yml).What this adds
Domain model. Two new tables.
savings_goals(UUID PK) belongs to a family and an asset account, has an AASM state machine (active/paused/completed/archived), monetisedtarget_amount, optionaltarget_date, color, notes;currencysyncs fromaccount.currencyviabefore_validationand locks once contributions exist.savings_contributionsbelongs to a goal and (forsource = 'auto'rows only) a budget. A custom validator requires the budget on auto rows; a partial unique index on(savings_goal_id, budget_id) WHERE source = 'auto'plus a CHECK constraint enforce auto-fund idempotency at the DB level. Currency on contributions syncs fromgoal.accountso they can't drift.Family#savings_summary_for(budget)returns a memoised value object (Family::SavingsSummaryviaData.define) carrying surplus / allocated / available / active_goals / currency, filtering active_goals tobudget.currencyso multi-currency families don't cross-fund.Budget#monthly_surpluscomputes[actual_income - actual_spending, 0].maxand lives next toavailable_to_allocate(kept public per @sokie's #833 review point that auto-fund called this off a private path).Background jobs.
SavingsGoals::AutoFundJobis the per-family worker: it acquires apg_advisory_xact_lockon the family (parameterised SQL viasanitize_sql_arrayso static analysis doesn't flag it), walks active goals alphabetically with one grouped balance prefetch up front, and caps each contribution bymin(monthly_target, remaining_pool, remaining_to_target). Each insert sits in its own savepoint (requires_new: true) so aRecordNotUniquefrom a concurrent worker doesn't poison the outer transaction.SavingsGoals::ScheduleAutoFundsJobis the cron orchestrator: it walks families with active goals in 1k-row batches, groups each batch by resolved month-start (so families with a non-defaultmonth_start_dayresolve their own boundary), and bulk-enqueues per-family jobs viaActiveJob.perform_all_later.Controllers / routes.
SavingsGoalsControllerhandles full CRUD plus AASM lifecycle members (pause/resume/complete/archive/unarchive). Family scoping is implicit viaCurrent.family.savings_goalssince Sure doesn't use Pundit.account_idis intentionally outside the permit list. It's read viaparams.dig(:savings_goal, :account_id)then run through a family-scopedlookup_account, so a foreign account id can never silently associate.SavingsContributionsController(nested under a goal) doesnew/create/destroy;createwraps inFamily.transaction { advisory_xact_lock }so concurrent "Add contribution" clicks serialize.Budgets::SavingsController#auto_fundis a single-action POST under the budget resource that enqueues the per-family job. Routes:/savings_goalsresource (with the lifecycle members and a:contributionssub-resource) plusPOST /budgets/:budget_month_year/savings/auto_fund.UI.
Savings::ProgressRingComponentis the SVG primitive with HTML-overlay text, the same pattern asapp/views/budgets/_budget_donut.html.erb, single-line by default, multi-line vialabel_lines:, stroke colours viavar(--color-…)so dark mode reflows automatically.Savings::GoalCardComponentis shared between the index and the budget summary card. All forms render inDS::Dialogviaframe: :modal, the same as the categories / tags / family_merchants pattern. The goal show page has a visible Edit pencil and a kebabDS::Menufor lifecycle and Delete, with items filtered by AASMmay_…?predicates. The color picker usesCategory::COLORSin a<details>reveal with a colored avatar preview that live-updates via a tiny declarative Stimulus controller (no Pickr dependency, no icon picker since goals are colour-only). All user-facing strings go throught()and live inconfig/locales/views/savings_goals/en.yml,config/locales/views/savings_contributions/en.yml, andconfig/locales/views/budgets/savings/en.yml. State labels undersavings_goals.states.*, dates viaI18n.l.Demo data.
Demo::Generator#generate_savings_goals!adds four goals against the demo family's depository accounts sorake demo_data:defaultexercises every state of the lifecycle: Awesome vacations (active / partially funded), Emergency fund (active / just started), Trip to Japan (active / empty), Paid-off car (completed). SameCategory::COLORSpalette.Architecture: choices vs PR #833
BudgetsController#show(a GET) into a Sidekiq job, keepingshowside-effect-free per @sokie's review point.SavingsGoalowns its math,Family#savings_summary_for(budget)returns the memoised struct,Budgetadds onlymonthly_surplus. NoBudget::SavingsIntegrationconcern, noapp/services/savings_goals/…./budgets/:month_yearitself (between the donut row and categories), not as a separate wizard-step or top-level nav item. Lines up with @jjmata's steer in Saving Goals #415 toward integration into Budgets without burying the feature.Reviewer-feedback checklist (PR #833)
BudgetsController#show(GET) into a background job + button (@sokie)_with_savingsmethods onBudget(@sokie)budget_idfamily-scoping is moot now, since the manual contribution form no longer exposesbudget_id; the auto path is family-scoped through the job (@sokie)family_id(@sokie)WHERE source = 'auto'and aRecordNotUniquerescue (@sokie)en.ymlfiles)iconhelper, never raw SVG, except insideSavings::ProgressRingComponentwhich is itself an SVG primitive (@sokie)SavingsContributionsControllertest coverage (@sokie)AutoFundJob: zero surplus, multi-goal competition, idempotency, race-RecordNotUnique, manual-vs-auto interaction, paused/completed exclusion, cron skip-when-no-budget (@sokie)var(--color-success)token; no heavy black background (@nuno-am)Category::COLORSand centred swatches via a<details>disclosure with live avatar preview (@nuno-am)Out of scope (intentional follow-ups)
These were considered and deliberately deferred to keep this PR focused. Happy to open follow-up issues for any of them.
/budgets/:month_yearand/savings_goalsfor now. A dashboard tile is a one-partial extension once a layout slot is picked.Api::V1::*Controller+ jbuilder +read/read_writescopes). We didn't pick a shape for/api/v1/savings_goals, the contributions sub-resource, or whetherauto_fundbelongs on the API surface. Better the maintainers weigh in first.guides/app-features/budgets.mdxand similar. Asavings-goals.mdxpage plus adocs.jsonnav entry are the natural follow-up; happy to open that PR once this one's direction is approved.en.yml: matches the repo norm (Budgeting V1, Nested Categories, Rules V1 all shipped English-only). Translation PRs land separately by locale contributors.current_balancesnapshot column: a denormalisedcurrent_balance_centsupdated viaafter_commitwould make goal reads zero-cost at the price of write amplification and eventual-consistency hazards. With per-instance memoisation and a grouped-SUM scope (both shipped here), the read cost is already negligible; defer until real numbers say otherwise.INSERT … ON CONFLICT DO NOTHINGin AutoFundJob: the savepoint-rescue pattern is correct and the file documents why; switching to ON CONFLICT shaves 2 round-trips per goal but is a big enough refactor to deserve its own PR.ActiveJob.perform_all_later. For very large operator deployments, you'd want to additionally spread enqueue times across the day; trivially done withset(wait: …)once that scale is real.target_date). Letting users set a manual monthly contribution for undated goals is a future design.Tests
bin/rails test: 3337 runs / 0 failures / 0 errors / 24 skips locally.bin/rubocop -f github,bundle exec erb_lint,bin/brakeman --no-pager --quiet,npm run lint, andnpm run format:checkall clean.bundle exec i18n-tasks healthconfirms the newsavings_goals/savings_contributions/budgets.savingskeys are present inen.yml; the project's existing 389k-missing-translations baseline already shows the same gap for many other features.bin/rails test:systemwas not added; see "Out of scope" above.Open questions for @jjmata
10 0 1 * *UTC (00:10 on the 1st of each month). OK with that schedule, or prefer manual-button-only for v1?SavingsGoal#account_idis non-null). Worth opening up to multiple accounts in a follow-up issue (Firefly III v2-style)? Or stays single-account?Test plan for reviewers
bin/setup&&rake demo_data:default. Log in asuser@example.com/Password1!.New goal. Modal opens. Pick an asset account, fill the form, optionally seed an initial contribution. Submit. Confirm you land on the new goal's show page.Archiveinstead ofMark complete.Auto-fund this monthon the budget card. Confirm the flash message and that contributions show up after Sidekiq processes the job./savings_goals, switch tabs throughAll / Active / Paused / Completed / Archived.