From d25bb6e2a3a0171abad3c01cfc62bd6bcdf21758 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 12 Apr 2026 14:17:38 +0000 Subject: [PATCH] Address all CodeRabbit PR review comments on insights feed - Add `Setting.insights_enabled` feature gate at top of GenerateInsightsJob#perform - Fix race condition in upsert_insight with recursive retry on RecordNotUnique - Use Provider::Openai.effective_model instead of hardcoded DEFAULT_MODEL - Rescue Money::Currency::UnknownCurrency in currency_symbol fallback - Fix budget dedup key to stable "budget_pacing:" (prevents flip-flop between at_risk/on_track) - Fix cash flow projection off-by-one: iterate (1..PROJECTION_DAYS) not (0..PROJECTION_DAYS) - Eliminate N+1 in IdleCashGenerator by precomputing active_account_ids in one SQL query - Rename all_time_high -> thirty_day_high (series is 30-day, not truly all-time) - Update net_worth_milestone i18n: title_ath -> title_30d_high, "30-day high" language - Anchor SpendingAnomalyGenerator baseline to current_period.start_date (not calendar months) - Fix spending_anomaly dedup key to use current_period.start_date instead of Date.current - Add .order(last_occurrence_date: :asc) to SubscriptionAuditGenerator for deterministic results - Stub Setting.insights_enabled in job tests; add generated_at assertion to upsert test https://claude.ai/code/session_014vY9xohpm3abSAxVxRF27a --- app/jobs/generate_insights_job.rb | 73 +++++++++++-------- app/models/insight/generator.rb | 6 +- .../generators/budget_insight_generator.rb | 2 +- .../generators/cash_flow_warning_generator.rb | 2 +- .../insight/generators/idle_cash_generator.rb | 16 ++-- .../net_worth_milestone_generator.rb | 33 ++++----- .../generators/spending_anomaly_generator.rb | 10 +-- .../subscription_audit_generator.rb | 1 + config/locales/views/insights/en.yml | 4 +- test/fixtures/insights.yml | 2 +- test/jobs/generate_insights_job_test.rb | 4 +- 11 files changed, 83 insertions(+), 70 deletions(-) diff --git a/app/jobs/generate_insights_job.rb b/app/jobs/generate_insights_job.rb index 90dc192c9..10694fe41 100644 --- a/app/jobs/generate_insights_job.rb +++ b/app/jobs/generate_insights_job.rb @@ -4,6 +4,8 @@ class GenerateInsightsJob < ApplicationJob # When called from cron (no family_id), iterates all families with insights enabled. # Can also be triggered on-demand for a single family (e.g., from the UI "Refresh" button). def perform(family_id: nil) + return unless Setting.insights_enabled + if family_id family = Family.find_by(id: family_id) generate_for_family(family) if family @@ -25,38 +27,45 @@ class GenerateInsightsJob < ApplicationJob def upsert_insights(family, generated_insights) generated_insights.each do |gi| - existing = family.insights.find_by(dedup_key: gi.dedup_key) - - if existing - # Reactivate only if the underlying numbers changed materially - numbers_changed = existing.metadata != gi.metadata - new_status = (numbers_changed && existing.dismissed?) ? "active" : existing.status - - existing.update!( - title: gi.title, - body: gi.body, - metadata: gi.metadata, - priority: gi.priority, - status: new_status, - generated_at: Time.current, - period_start: gi.period_start, - period_end: gi.period_end - ) - else - family.insights.create!( - insight_type: gi.insight_type, - priority: gi.priority, - status: "active", - title: gi.title, - body: gi.body, - metadata: gi.metadata, - currency: gi.currency || family.currency, - period_start: gi.period_start, - period_end: gi.period_end, - dedup_key: gi.dedup_key, - generated_at: Time.current - ) - end + upsert_insight(family, gi) end end + + def upsert_insight(family, gi, attempts: 0) + existing = family.insights.find_by(dedup_key: gi.dedup_key) + + if existing + # Reactivate only if the underlying numbers changed materially + numbers_changed = existing.metadata != gi.metadata + new_status = (numbers_changed && existing.dismissed?) ? "active" : existing.status + + existing.update!( + title: gi.title, + body: gi.body, + metadata: gi.metadata, + priority: gi.priority, + status: new_status, + generated_at: Time.current, + period_start: gi.period_start, + period_end: gi.period_end + ) + else + family.insights.create!( + insight_type: gi.insight_type, + priority: gi.priority, + status: "active", + title: gi.title, + body: gi.body, + metadata: gi.metadata, + currency: gi.currency || family.currency, + period_start: gi.period_start, + period_end: gi.period_end, + dedup_key: gi.dedup_key, + generated_at: Time.current + ) + end + rescue ActiveRecord::RecordNotUnique + raise if attempts >= 2 + upsert_insight(family, gi, attempts: attempts + 1) + end end diff --git a/app/models/insight/generator.rb b/app/models/insight/generator.rb index 01fd0f49b..c51f44567 100644 --- a/app/models/insight/generator.rb +++ b/app/models/insight/generator.rb @@ -38,7 +38,7 @@ class Insight::Generator response = llm.chat_response( prompt, - model: Provider::Openai::DEFAULT_MODEL, + model: Provider::Openai.effective_model, instructions: system_instructions ) @@ -49,7 +49,7 @@ class Insight::Generator end def system_instructions - sym = Money::Currency.new(family.currency).symbol + sym = currency_symbol <<~PROMPT You are a concise financial insights writer for a personal finance app. Write exactly 1-2 sentences in plain, conversational English. @@ -60,5 +60,7 @@ class Insight::Generator def currency_symbol Money::Currency.new(family.currency).symbol + rescue Money::Currency::UnknownCurrency + family.currency end end diff --git a/app/models/insight/generators/budget_insight_generator.rb b/app/models/insight/generators/budget_insight_generator.rb index 80ee656b3..1c46c7f09 100644 --- a/app/models/insight/generators/budget_insight_generator.rb +++ b/app/models/insight/generators/budget_insight_generator.rb @@ -62,7 +62,7 @@ class Insight::Generators::BudgetInsightGenerator < Insight::Generator currency: family.currency, period_start: current_period.start_date, period_end: current_period.end_date, - dedup_key: "#{insight_type}:#{current_period.start_date.strftime("%Y-%m")}" + dedup_key: "budget_pacing:#{current_period.start_date.strftime("%Y-%m-%d")}" ) ] end diff --git a/app/models/insight/generators/cash_flow_warning_generator.rb b/app/models/insight/generators/cash_flow_warning_generator.rb index 912a06bcb..e6f533092 100644 --- a/app/models/insight/generators/cash_flow_warning_generator.rb +++ b/app/models/insight/generators/cash_flow_warning_generator.rb @@ -59,7 +59,7 @@ class Insight::Generators::CashFlowWarningGenerator < Insight::Generator balance = current_cash balance_by_date = {} - (0..PROJECTION_DAYS).each do |day_offset| + (1..PROJECTION_DAYS).each do |day_offset| date = Date.current + day_offset events.each do |event| diff --git a/app/models/insight/generators/idle_cash_generator.rb b/app/models/insight/generators/idle_cash_generator.rb index 8fe72f6e9..d59ebfaac 100644 --- a/app/models/insight/generators/idle_cash_generator.rb +++ b/app/models/insight/generators/idle_cash_generator.rb @@ -7,17 +7,17 @@ class Insight::Generators::IdleCashGenerator < Insight::Generator def generate depository_accounts = family.accounts.visible.assets.where(accountable_type: "Depository") - depository_accounts.filter_map do |account| + active_account_ids = Entry + .where(account_id: depository_accounts.select(:id)) + .where("date >= ?", IDLE_THRESHOLD_DAYS.days.ago.to_date) + .where(entryable_type: "Transaction") + .distinct + .pluck(:account_id) + + depository_accounts.where.not(id: active_account_ids).filter_map do |account| balance = account.balance.to_f next unless balance >= IDLE_AMOUNT_THRESHOLD - recent_activity = account.entries - .where("date >= ?", IDLE_THRESHOLD_DAYS.days.ago.to_date) - .where(entryable_type: "Transaction") - .exists? - - next if recent_activity - metadata = { "account_id" => account.id, "account_name" => account.name, diff --git a/app/models/insight/generators/net_worth_milestone_generator.rb b/app/models/insight/generators/net_worth_milestone_generator.rb index cb0be4947..a8fd38e8c 100644 --- a/app/models/insight/generators/net_worth_milestone_generator.rb +++ b/app/models/insight/generators/net_worth_milestone_generator.rb @@ -1,5 +1,4 @@ -# Surfaces an insight when net worth crosses a round-number milestone or hits an all-time high -# over the past 30 days. +# Surfaces an insight when net worth crosses a round-number milestone or hits a 30-day high. class Insight::Generators::NetWorthMilestoneGenerator < Insight::Generator ROUND_MILESTONES = [ 1_000, 5_000, 10_000, 25_000, 50_000, @@ -21,32 +20,32 @@ class Insight::Generators::NetWorthMilestoneGenerator < Insight::Generator prior_values = extract_prior_values(prior_series) return [] if prior_values.empty? - prior_nw = prior_values.last.to_f - series_high = prior_values.max.to_f - all_time_high = current_nw >= series_high && current_nw > prior_nw + prior_nw = prior_values.last.to_f + series_high = prior_values.max.to_f + thirty_day_high = current_nw >= series_high && current_nw > prior_nw crossed = ROUND_MILESTONES.find { |m| current_nw >= m && prior_nw < m } - return [] unless crossed || all_time_high + return [] unless crossed || thirty_day_high metadata = { "milestone" => crossed, "current_net_worth" => current_nw.round(2), "previous_net_worth" => prior_nw.round(2), - "all_time_high" => all_time_high + "thirty_day_high" => thirty_day_high } - title = if all_time_high && crossed + title = if thirty_day_high && crossed I18n.t("insights.net_worth_milestone.title_both", milestone: format_amount(crossed)) - elsif all_time_high - I18n.t("insights.net_worth_milestone.title_ath") + elsif thirty_day_high + I18n.t("insights.net_worth_milestone.title_30d_high") else I18n.t("insights.net_worth_milestone.title_milestone", milestone: format_amount(crossed)) end - body = generate_body(build_prompt(crossed, all_time_high, current_nw, prior_nw)) + body = generate_body(build_prompt(crossed, thirty_day_high, current_nw, prior_nw)) [ GeneratedInsight.new( @@ -58,7 +57,7 @@ class Insight::Generators::NetWorthMilestoneGenerator < Insight::Generator currency: family.currency, period_start: prior_period.start_date, period_end: Date.current, - dedup_key: "net_worth_milestone:#{crossed || "ath"}:#{Date.current.strftime("%Y-%m")}" + dedup_key: "net_worth_milestone:#{crossed || "30d_high"}:#{Date.current.strftime("%Y-%m")}" ) ] end @@ -73,15 +72,15 @@ class Insight::Generators::NetWorthMilestoneGenerator < Insight::Generator end end - def build_prompt(crossed, all_time_high, current_nw, prior_nw) + def build_prompt(crossed, thirty_day_high, current_nw, prior_nw) sym = currency_symbol current_fmt = "#{sym}#{current_nw.round(0).to_s(:delimited)}" prior_fmt = "#{sym}#{prior_nw.round(0).to_s(:delimited)}" - if all_time_high && crossed - "Net worth crossed #{sym}#{crossed.to_s(:delimited)} and hit an all-time high of #{current_fmt}." - elsif all_time_high - "Net worth hit an all-time high of #{current_fmt}, up from #{prior_fmt} 30 days ago." + if thirty_day_high && crossed + "Net worth crossed #{sym}#{crossed.to_s(:delimited)} and hit a 30-day high of #{current_fmt}." + elsif thirty_day_high + "Net worth hit a 30-day high of #{current_fmt}, up from #{prior_fmt} 30 days ago." else "Net worth crossed the #{sym}#{crossed.to_s(:delimited)} milestone, now at #{current_fmt}." end diff --git a/app/models/insight/generators/spending_anomaly_generator.rb b/app/models/insight/generators/spending_anomaly_generator.rb index 866b435f3..0c0e14a30 100644 --- a/app/models/insight/generators/spending_anomaly_generator.rb +++ b/app/models/insight/generators/spending_anomaly_generator.rb @@ -5,11 +5,11 @@ class Insight::Generators::SpendingAnomalyGenerator < Insight::Generator MIN_BASELINE_SPEND = 50 # Ignore tiny categories (noise reduction) def generate - baseline_period = Period.custom( - start_date: 3.months.ago.to_date.beginning_of_month, - end_date: 1.month.ago.to_date.end_of_month - ) current_period = Period.current_month_for(family) + baseline_period = Period.custom( + start_date: current_period.start_date - 3.months, + end_date: current_period.start_date - 1.day + ) income_stmt = family.income_statement @@ -79,7 +79,7 @@ class Insight::Generators::SpendingAnomalyGenerator < Insight::Generator currency: family.currency, period_start: current_period.start_date, period_end: current_period.end_date, - dedup_key: "spending_anomaly:#{cat_id}:#{Date.current.strftime("%Y-%m")}" + dedup_key: "spending_anomaly:#{cat_id}:#{current_period.start_date.strftime("%Y-%m-%d")}" ) end diff --git a/app/models/insight/generators/subscription_audit_generator.rb b/app/models/insight/generators/subscription_audit_generator.rb index 1ff465645..771823716 100644 --- a/app/models/insight/generators/subscription_audit_generator.rb +++ b/app/models/insight/generators/subscription_audit_generator.rb @@ -10,6 +10,7 @@ class Insight::Generators::SubscriptionAuditGenerator < Insight::Generator .where("last_occurrence_date < ?", OVERDUE_DAYS.days.ago.to_date) .where("next_expected_date < ?", Date.current) .includes(:merchant) + .order(last_occurrence_date: :asc) .limit(5) return [] if stale.empty? diff --git a/config/locales/views/insights/en.yml b/config/locales/views/insights/en.yml index ef8e634e0..a281c171d 100644 --- a/config/locales/views/insights/en.yml +++ b/config/locales/views/insights/en.yml @@ -22,9 +22,9 @@ en: cash_flow_warning: title: Low cash projected around %{date} net_worth_milestone: - title_ath: New all-time high net worth + title_30d_high: New 30-day high net worth title_milestone: Net worth crossed %{milestone} - title_both: Net worth crossed %{milestone} — new all-time high + title_both: Net worth crossed %{milestone} — new 30-day high subscription_audit: title: Is %{name} still active? savings_rate_change: diff --git a/test/fixtures/insights.yml b/test/fixtures/insights.yml index 73017e6f6..fa95aee36 100644 --- a/test/fixtures/insights.yml +++ b/test/fixtures/insights.yml @@ -48,7 +48,7 @@ net_worth_milestone: milestone: 100000 current_net_worth: 101342.00 previous_net_worth: 98800.00 - all_time_high: true + thirty_day_high: true currency: USD period_start: <%= 30.days.ago.to_date %> period_end: <%= Date.current %> diff --git a/test/jobs/generate_insights_job_test.rb b/test/jobs/generate_insights_job_test.rb index 8dd708fd8..0a61c1303 100644 --- a/test/jobs/generate_insights_job_test.rb +++ b/test/jobs/generate_insights_job_test.rb @@ -3,6 +3,7 @@ require "test_helper" class GenerateInsightsJobTest < ActiveJob::TestCase setup do @family = families(:dylan_family) + Setting.stubs(:insights_enabled).returns(true) end test "performs without error for a family with no accounts" do @@ -46,7 +47,7 @@ class GenerateInsightsJobTest < ActiveJob::TestCase test "updates existing insight body and generated_at on repeated runs" do dedup = "net_worth_milestone:update_test:#{Date.current.strftime("%Y-%m")}" - @family.insights.create!( + old_insight = @family.insights.create!( insight_type: "net_worth_milestone", priority: "high", status: "active", @@ -78,5 +79,6 @@ class GenerateInsightsJobTest < ActiveJob::TestCase refreshed = @family.insights.find_by(dedup_key: dedup) assert_equal "New title", refreshed.title assert_equal "New body", refreshed.body + assert refreshed.generated_at > old_insight.generated_at, "generated_at should be updated on upsert" end end