From d6a12614a7881e5f2f5d67083d6f1fdc794437ff Mon Sep 17 00:00:00 2001 From: Guillem Arias Date: Fri, 15 May 2026 00:16:54 +0200 Subject: [PATCH] fix(goals): address second AI review round on PR #1798 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Parse "YYYY-MM-DD" date-only strings as local midnight in the projection chart so users west of UTC stop seeing the today marker and hover dates land one calendar day back - Order the demo-generator depository pickup by (created_at, id) so primary/secondary roles stay stable across reseeds and the state matrix (behind / on_track / reached / no_target_date / past-due) surfaces the same goals every time - Drop the brittle " · "-split on goals.goal_card.days_left in Goal#header_summary (the translation has no separator suffix) - Goal#projection_payload ships pre-formatted strings for the static chart annotations (target_amount_label / short, projection_end_label, projection_shortfall_label, pending_pledge_label_short) and the controller now renders those instead of running Intl.NumberFormat on each draw. Y-axis tick labels stay JS-side because they depend on D3's dynamically-chosen tick values. --- .../goal_projection_chart_controller.js | 51 +++++++++++-------- app/models/demo/generator.rb | 9 +++- app/models/goal.rb | 48 +++++++++++++++-- 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/app/javascript/controllers/goal_projection_chart_controller.js b/app/javascript/controllers/goal_projection_chart_controller.js index bdb791ac8..f7b02107c 100644 --- a/app/javascript/controllers/goal_projection_chart_controller.js +++ b/app/javascript/controllers/goal_projection_chart_controller.js @@ -81,9 +81,18 @@ export default class extends Controller { const innerWidth = width - margin.left - margin.right; const innerHeight = height - margin.top - margin.bottom; - const start = new Date(data.start_date); - const today = new Date(data.today); - const target = data.target_date ? new Date(data.target_date) : null; + // Date-only payload strings ("YYYY-MM-DD") parse as UTC midnight in + // `new Date(str)`, which shifts displayed days back one for users west + // of Greenwich. Parse components so today/target/saved_series sit on + // local-midnight. + const parseLocalDate = (s) => { + if (!s) return null; + const [ y, m, d ] = s.split("-").map(Number); + return new Date(y, m - 1, d); + }; + const start = parseLocalDate(data.start_date); + const today = parseLocalDate(data.today); + const target = parseLocalDate(data.target_date); const targetAmount = data.target_amount || 0; const currentAmount = data.current_amount || 0; const avgMonthly = data.avg_monthly || 0; @@ -100,7 +109,7 @@ export default class extends Controller { // Without this, the snapshot in `balances` for today could differ from // the live read (sync timing) and the chart showed a vertical jump. const rawSavedSeries = (data.saved_series || []) - .map((p) => ({ date: new Date(p.date), value: p.value })) + .map((p) => ({ date: parseLocalDate(p.date), value: p.value })) .filter((p) => p.date < today); const firstContribDate = rawSavedSeries[0]?.date; const savedSeries = []; @@ -219,7 +228,7 @@ export default class extends Controller { .attr("text-anchor", "end") .attr("font-size", 12) .attr("fill", textPrimary) - .text(`Target · ${this._fmtMoneyShort(targetAmount, data.currency)}`); + .text(`Target · ${data.target_amount_short_label}`); } else { // Plenty of room: keep the right-side full-format label. svg @@ -229,7 +238,7 @@ export default class extends Controller { .attr("text-anchor", "end") .attr("font-size", 12) .attr("fill", textPrimary) - .text(`Target · ${this._fmtMoney(targetAmount, data.currency)}`); + .text(`Target · ${data.target_amount_label}`); } } @@ -309,20 +318,22 @@ export default class extends Controller { const collidesWithTargetLabel = targetAmount > 0 && Math.abs(projDotY - y(targetAmount)) < 18; if (innerWidth >= 320 && !(willHit && collidesWithTargetLabel)) { - // Full Intl.NumberFormat (no K/M shorthand) so the chart annotation - // matches the rest of the page's monetary readouts ("$160,634 - // short" reads cleanly next to "$26,621/mo to catch up"). + // Server-rendered labels: projection_end_label is the full-format + // currency for the on-track endpoint, projection_shortfall_label + // is the "$X short" string when we fall short. const labelText = willHit - ? this._fmtMoney(projectionEnd, data.currency) - : `${this._fmtMoney(targetAmount - projectionEnd, data.currency)} short`; - svg - .append("text") - .attr("x", x(target) - 8) - .attr("y", y(projectionEnd) - 8) - .attr("text-anchor", "end") - .attr("font-size", 12) - .attr("fill", textSecondary) - .text(labelText); + ? data.projection_end_label + : (data.projection_shortfall_label ? `${data.projection_shortfall_label} short` : ""); + if (labelText) { + svg + .append("text") + .attr("x", x(target) - 8) + .attr("y", y(projectionEnd) - 8) + .attr("text-anchor", "end") + .attr("font-size", 12) + .attr("fill", textSecondary) + .text(labelText); + } } } @@ -358,7 +369,7 @@ export default class extends Controller { .attr("y", y(pendingTop) + 4) .attr("font-size", 12) .attr("fill", textSecondary) - .text(`+ pending ${this._fmtMoneyShort(pendingPledgeAmount, data.currency)}`); + .text(`+ pending ${data.pending_pledge_label_short}`); } } diff --git a/app/models/demo/generator.rb b/app/models/demo/generator.rb index f7c317148..87965c72a 100644 --- a/app/models/demo/generator.rb +++ b/app/models/demo/generator.rb @@ -1279,7 +1279,14 @@ class Demo::Generator end def generate_goals!(family) - depository_accounts = family.accounts.where(accountable_type: "Depository").visible.to_a + # Order so primary/secondary picks stay stable across reseeds — the + # state-coverage matrix below routes goals to specific accounts to + # surface every status branch, so DB return order can't pick. + depository_accounts = family.accounts + .where(accountable_type: "Depository") + .visible + .order(:created_at, :id) + .to_a return if depository_accounts.empty? currency = depository_accounts.first.currency diff --git a/app/models/goal.rb b/app/models/goal.rb index 8274a1778..65937ef14 100644 --- a/app/models/goal.rb +++ b/app/models/goal.rb @@ -179,28 +179,50 @@ class Goal < ApplicationRecord # 90-day balance trajectory of linked accounts. Used by the projection chart # to render the saved-to-date line. Returns an empty series when the linked - # account lacks ≥30 days of history. + # account lacks ≥30 days of history. Ships pre-formatted labels for the + # static chart annotations (target line, projection-end / shortfall, + # pending-pledge badge) so the Stimulus controller only has to render + # strings server-side rather than build them with its own Intl calls. def projection_payload series_values = balance_series_values saved_series = series_values.map { |v| { date: v.date.to_s, value: v.value.amount.to_f } } earliest = series_values.first&.date || created_at.to_date + pending = open_pledges.sum(:amount).to_d + target_amt = target_amount.to_d + proj_end = projection_end_amount { saved_series: saved_series, start_date: earliest.to_s, today: Date.current.to_s, target_date: target_date&.to_s, - target_amount: target_amount.to_f, + target_amount: target_amt.to_f, + target_amount_label: Money.new(target_amt, currency).format(precision: 0), + target_amount_short_label: short_money(target_amt, currency), current_amount: current_balance.to_f, avg_monthly: pace.to_f, required_monthly: monthly_target_amount.to_f, currency: currency, status: status.to_s, - pending_pledge_amount: open_pledges.sum(:amount).to_f + pending_pledge_amount: pending.to_f, + pending_pledge_label_short: short_money(pending, currency), + projection_end_value: proj_end.to_f, + projection_end_label: Money.new(proj_end, currency).format(precision: 0), + projection_shortfall_label: (target_amt > proj_end ? Money.new(target_amt - proj_end, currency).format(precision: 0) : nil) } end + # Projected balance at the target_date given the current pace. Mirrors + # the JS calculation so the server can pre-format the chart annotation + # without re-rendering after each Stimulus draw. + def projection_end_amount + return current_balance.to_d if target_date.nil? + months = ((target_date - Date.current).to_f / 30.44).clamp(0.0, Float::INFINITY) + projected = current_balance.to_d + (pace.to_d * months) + [ current_balance.to_d, projected ].max + end + def display_status return @display_status if defined?(@display_status) @@ -300,7 +322,7 @@ class Goal < ApplicationRecord amount: target_amount_money.format(precision: 0), date: I18n.l(target_date, format: :long)) if days > 0 && !(completed? || status == :reached) - parts << I18n.t("goals.goal_card.days_left", count: days).split(" · ").first + parts << I18n.t("goals.goal_card.days_left", count: days) end end else @@ -349,6 +371,24 @@ class Goal < ApplicationRecord end private + # K/M shorthand for narrow chart annotations (axis ticks, projection + # short-form, pending-pledge badge). Locale-aware currency symbol via + # Money so the chart matches the rest of the app for EUR/GBP families. + def short_money(amount, code) + amount_f = amount.to_f + symbol = Money.new(0, code).symbol + abs = amount_f.abs + if abs >= 1_000_000 + short = (amount_f / 1_000_000.0).round(1) + "#{symbol}#{short == short.to_i ? short.to_i : short}M" + elsif abs >= 1_000 + short = (amount_f / 1_000.0).round(1) + "#{symbol}#{short == short.to_i ? short.to_i : short}K" + else + "#{symbol}#{amount_f.round.to_i.to_s.reverse.gsub(/(\d{3})(?=\d)/, '\1,').reverse}" + end + end + def balance_series_values return [] if linked_accounts.empty?