From bc7e16ff167c0e8365f4660e8a8db6351cbe0020 Mon Sep 17 00:00:00 2001 From: William Wei Ming <280573057+bittensorrider@users.noreply.github.com> Date: Sun, 31 May 2026 06:09:38 +0800 Subject: [PATCH] Perf/dashboard endpoint optimization (#1897) * optimize net_category_totals() by using memoized cache * fix issue - net_category_totals cache is never populated - suggested by coderabbitAI * fix 422 error for service-worker * remove warning of [assigned but unused variables] - income_statement.rb * remove warnings of [assigned but unused] from Prism - income_statement_test.rb * add some measurements to improve docstring coverage, follow CodeRabbit recommendation * attach Skylight monitoring for dev env as well - use my own Skylight auth token * integrate Skylight with my own account auth token for local benchmark * fix PR review suggestion - Move fallback release-note copy to i18n keys * follow PR review - Fix changelog GitHub fetch timeout bounding * FIX - Variable shadowing; Prefer stubbing the specific instance over any_instance.expects * fix CodeRabbit feedback - Reusing the same stub for both classifications hides a contract mismatch * fix CodeRabbit FEEDBACK - Reconsider enabling Skylight by default in development * fix CodeRabbitAI FEEDBACK - reconsider unconditionally enabling Skylight in development * fix Security scan FEEDBACK before PR merge * fix jjmata feedback --- .env.example | 5 ++++ .env.local.example | 5 ++++ Gemfile | 6 ++++- app/controllers/pages_controller.rb | 4 +-- app/models/income_statement.rb | 24 ++++++++--------- config/application.rb | 8 +++++- config/initializers/mime_types.rb | 3 +++ config/locales/views/pages/en.yml | 3 +++ config/skylight.yml | 1 + test/controllers/pages_controller_test.rb | 33 +++++++++++++++++++++++ test/models/income_statement_test.rb | 30 ++++++++++++++++----- 11 files changed, 99 insertions(+), 23 deletions(-) create mode 100644 config/skylight.yml diff --git a/.env.example b/.env.example index e0bc454b1..9128d758a 100644 --- a/.env.example +++ b/.env.example @@ -188,3 +188,8 @@ POSTHOG_HOST= # GCS_BUCKET= # GCS_KEYFILE_JSON= <- JSON content of service account key (preferred) # GCS_KEYFILE= <- path to service account JSON key file + +# Skylight +# ======== +SKYLIGHT_AUTHENTICATION= +SKYLIGHT_ENABLED= diff --git a/.env.local.example b/.env.local.example index cbe138774..fea16335c 100644 --- a/.env.local.example +++ b/.env.local.example @@ -107,3 +107,8 @@ AI_DEBUG_MODE = # GCS_BUCKET= # GCS_KEYFILE_JSON= # GCS_KEYFILE= + +# Skylight +# ======== +SKYLIGHT_AUTHENTICATION= +SKYLIGHT_ENABLED= diff --git a/Gemfile b/Gemfile index 19fdf8a17..0af4a74a2 100644 --- a/Gemfile +++ b/Gemfile @@ -44,7 +44,11 @@ gem "sentry-rails" gem "sentry-sidekiq" gem "posthog-ruby" gem "logtail-rails" -gem "skylight", groups: [ :production ] +if ENV["SKYLIGHT_ENABLED"] == "true" + gem "skylight", group: :development, require: false +else + gem "skylight", group: :production +end # Active Storage gem "aws-sdk-s3", "~> 1.208.0", require: false diff --git a/app/controllers/pages_controller.rb b/app/controllers/pages_controller.rb index c52b7a883..90a1f1054 100644 --- a/app/controllers/pages_controller.rb +++ b/app/controllers/pages_controller.rb @@ -49,9 +49,9 @@ class PagesController < ApplicationController @release_notes = { avatar: "https://github.com/we-promise.png", username: "we-promise", - name: "Release notes unavailable", + name: t("pages.release_notes_unavailable.name"), published_at: Date.current, - body: "
Unable to fetch the latest release notes at this time. Please check back later or visit our GitHub releases page directly.
" + body: t("pages.release_notes_unavailable.body_html") } end diff --git a/app/models/income_statement.rb b/app/models/income_statement.rb index 6082f8e74..dd1cf459a 100644 --- a/app/models/income_statement.rb +++ b/app/models/income_statement.rb @@ -31,21 +31,23 @@ class IncomeStatement def expense_totals(period: Period.current_month) # Memoized per instance so callers that also invoke `net_category_totals` + key = period_cache_key(period) @expense_totals_by_period ||= {} - @expense_totals_by_period[period_cache_key(period)] ||= - build_period_total(classification: "expense", period: period) + return @expense_totals_by_period[key] if @expense_totals_by_period.key?(key) + @expense_totals_by_period[key] = build_period_total(classification: "expense", period: period) end def income_totals(period: Period.current_month) + key = period_cache_key(period) @income_totals_by_period ||= {} - @income_totals_by_period[period_cache_key(period)] ||= - build_period_total(classification: "income", period: period) + return @income_totals_by_period[key] if @income_totals_by_period.key?(key) + @income_totals_by_period[key] = build_period_total(classification: "income", period: period) end def net_category_totals(period: Period.current_month) + key = period_cache_key(period) @net_category_totals_by_period ||= {} - cached = @net_category_totals_by_period[period_cache_key(period)] - return cached if cached + return @net_category_totals_by_period[key] if @net_category_totals_by_period.key?(key) expense = expense_totals(period: period) income = income_totals(period: period) @@ -68,9 +70,9 @@ class IncomeStatement raw_expense_categories = [] raw_income_categories = [] - all_keys.each do |key| - exp_ct = expense_by_cat[key] - inc_ct = income_by_cat[key] + all_keys.each do |cat| + exp_ct = expense_by_cat[cat] + inc_ct = income_by_cat[cat] exp_total = exp_ct&.total || 0 inc_total = inc_ct&.total || 0 net = exp_total - inc_total @@ -96,7 +98,7 @@ class IncomeStatement CategoryTotal.new(category: r[:category], total: r[:total], currency: family.currency, weight: weight) end - @net_category_totals_by_period[period_cache_key(period)] = NetCategoryTotals.new( + @net_category_totals_by_period[key] = NetCategoryTotals.new( net_expense_categories: net_expense_categories, net_income_categories: net_income_categories, total_net_expense: total_net_expense, @@ -148,8 +150,6 @@ class IncomeStatement other_investments_category = family.categories.other_investments category_totals = [ *categories, uncategorized_category, other_investments_category ].map do |category| - subcategory = categories.find { |c| c.id == category.parent_id } - parent_category_total = if category.uncategorized? # Regular uncategorized: NULL category_id and NOT uncategorized investment totals.select { |t| t.category_id.nil? && !t.is_uncategorized_investment }&.sum(&:total) || 0 diff --git a/config/application.rb b/config/application.rb index 17909aadf..cba92a83c 100644 --- a/config/application.rb +++ b/config/application.rb @@ -40,7 +40,13 @@ module Sure } # Enable Skylight instrumentation for ActiveJob (background workers) - config.skylight.probes << "active_job" if defined?(Skylight) + # Developers can opt-in to Skylight locally by setting SKYLIGHT_ENABLED=true + if defined?(Skylight) && config.respond_to?(:skylight) + config.skylight.probes << "active_job" + if ENV["SKYLIGHT_ENABLED"] == "true" + config.skylight.environments += [ "development" ] + end + end # Enable Rack::Attack middleware for API rate limiting config.middleware.use Rack::Attack diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index e116f5753..cb66cb540 100644 --- a/config/initializers/mime_types.rb +++ b/config/initializers/mime_types.rb @@ -8,3 +8,6 @@ Mime::Type.register "application/vnd.openxmlformats-officedocument.spreadsheetml # Register .mjs so Propshaft serves ES modules with the correct Content-Type. Mime::Type.register "text/javascript", :mjs unless Mime::Type.lookup_by_extension(:mjs) + +# Service Worker +Mime::Type.register "application/javascript", :serviceworker diff --git a/config/locales/views/pages/en.yml b/config/locales/views/pages/en.yml index df0a8a70c..b03062678 100644 --- a/config/locales/views/pages/en.yml +++ b/config/locales/views/pages/en.yml @@ -26,6 +26,9 @@ en: refresh_page: Refresh Page changelog: title: What's new + release_notes_unavailable: + name: Release notes unavailable + body_html: "Unable to fetch the latest release notes at this time. Please check back later or visit our GitHub releases page directly.
" privacy: title: Privacy Policy heading: Privacy Policy diff --git a/config/skylight.yml b/config/skylight.yml new file mode 100644 index 000000000..9b33a6bcd --- /dev/null +++ b/config/skylight.yml @@ -0,0 +1 @@ +authentication: <%= ENV["SKYLIGHT_AUTHENTICATION"] %> diff --git a/test/controllers/pages_controller_test.rb b/test/controllers/pages_controller_test.rb index 023a983f9..8dcef79b9 100644 --- a/test/controllers/pages_controller_test.rb +++ b/test/controllers/pages_controller_test.rb @@ -14,6 +14,39 @@ class PagesControllerTest < ActionDispatch::IntegrationTest assert_response :ok end + test "dashboard memoizes income statement period totals while rendering" do + income_statement = IncomeStatement.new(@family) + IncomeStatement.stubs(:new).returns(income_statement) + + fake_expense_period_total = IncomeStatement::PeriodTotal.new( + classification: "expense", + total: 0, + currency: @family.currency, + category_totals: [] + ) + + fake_income_period_total = IncomeStatement::PeriodTotal.new( + classification: "income", + total: 0, + currency: @family.currency, + category_totals: [] + ) + + income_statement.expects(:build_period_total) + .with(classification: "expense", period: kind_of(Period)) + .once + .returns(fake_expense_period_total) + + income_statement.expects(:build_period_total) + .with(classification: "income", period: kind_of(Period)) + .once + .returns(fake_income_period_total) + + get root_path + + assert_response :ok + end + test "intro page requires guest role" do get intro_path diff --git a/test/models/income_statement_test.rb b/test/models/income_statement_test.rb index f1dc44435..2752a46a8 100644 --- a/test/models/income_statement_test.rb +++ b/test/models/income_statement_test.rb @@ -39,6 +39,22 @@ class IncomeStatementTest < ActiveSupport::TestCase assert_equal expected_total_expense, expense_totals.category_totals.find { |ct| ct.category.id == @food_category.id }.total end + test "memoizes expense and income period totals across repeated calculations" do + income_statement = IncomeStatement.new(@family) + period = Period.last_30_days + + expense_period_total = IncomeStatement::PeriodTotal.new("expense", 900, @family.currency, []) + income_period_total = IncomeStatement::PeriodTotal.new("income", 1000, @family.currency, []) + + income_statement.expects(:build_period_total).with(classification: "expense", period: period).once.returns(expense_period_total) + income_statement.expects(:build_period_total).with(classification: "income", period: period).once.returns(income_period_total) + + income_statement.net_category_totals(period: period) + income_statement.expense_totals(period: period) + income_statement.income_totals(period: period) + income_statement.net_category_totals(period: period) + end + test "calculates income for a period" do income_statement = IncomeStatement.new(@family) income_totals = income_statement.income_totals(period: Period.last_30_days) @@ -154,8 +170,8 @@ class IncomeStatementTest < ActiveSupport::TestCase # NOTE: These tests now pass because kind filtering is working after the refactoring! test "excludes regular transfers from income statement calculations" do # Create a regular transfer between accounts - outflow_transaction = create_transaction(account: @checking_account, amount: 500, kind: "funds_movement") - inflow_transaction = create_transaction(account: @credit_card_account, amount: -500, kind: "funds_movement") + _outflow_transaction = create_transaction(account: @checking_account, amount: 500, kind: "funds_movement") + _inflow_transaction = create_transaction(account: @credit_card_account, amount: -500, kind: "funds_movement") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -168,7 +184,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "includes loan payments as expenses in income statement" do # Create a loan payment transaction - loan_payment = create_transaction(account: @checking_account, amount: 1000, category: nil, kind: "loan_payment") + _loan_payment = create_transaction(account: @checking_account, amount: 1000, category: nil, kind: "loan_payment") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -181,7 +197,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "excludes one-time transactions from income statement calculations" do # Create a one-time transaction - one_time_transaction = create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time") + _one_time_transaction = create_transaction(account: @checking_account, amount: 250, category: @groceries_category, kind: "one_time") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -194,7 +210,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "excludes payment transactions from income statement calculations" do # Create a payment transaction (credit card payment) - payment_transaction = create_transaction(account: @checking_account, amount: 300, category: nil, kind: "cc_payment") + _payment_transaction = create_transaction(account: @checking_account, amount: 300, category: nil, kind: "cc_payment") income_statement = IncomeStatement.new(@family) totals = income_statement.totals(date_range: Period.last_30_days.date_range) @@ -288,7 +304,7 @@ class IncomeStatementTest < ActiveSupport::TestCase test "includes investment_contribution transactions as expenses in income statement" do # Create a transfer to investment account (marked as investment_contribution) - investment_contribution = create_transaction( + _investment_contribution = create_transaction( account: @checking_account, amount: 1000, category: nil, @@ -318,7 +334,7 @@ class IncomeStatementTest < ActiveSupport::TestCase # Provider-imported contribution shows as inflow (negative amount) to the investment account # kind is investment_contribution, which should be treated as expense regardless of sign - provider_contribution = create_transaction( + _provider_contribution = create_transaction( account: investment_account, amount: -500, # Negative = inflow to account category: nil,