From 7e1de420cae7ddd7f39e31362916133e087364e2 Mon Sep 17 00:00:00 2001 From: GermanDZ Date: Thu, 7 May 2026 17:31:16 +0200 Subject: [PATCH] perf(accounts): kill sidebar/sparkline N+1s and cache the sidebar (#1683) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(accounts): kill sidebar/sparkline N+1s and cache the sidebar The dashboard was issuing hundreds of per-account `SELECT 1` and polymorphic `accountable` lookups on every page load. Sidebar render alone hit the DB ~50–100× and ran twice per request (mobile + desktop). Changes: - AccountableSparklinesController: short-circuit `requires_normalized_aggregation?` to Investment/Crypto only and collapse the per-account `linked?` loop into a single `EXISTS`. Kills the N+1 `AccountProvider Exists?` queries on every sparkline endpoint. - BalanceSheet::AccountTotals#visible_accounts: preload `:accountable`, `:plaid_account`, `:simplefin_account`, and `account_providers: :provider` so the sidebar's `account.subtype` / `account.linked?` / `account.provider` calls don't trigger per-row polymorphic loads. - AccountsController#index: same preloads on `@manual_accounts`. - accounts/index/_account_groups.erb: extend the existing `Preloader` call to batch-load accountable + provider associations so the per-provider-item partials (Plaid, SimpleFIN, Coinbase, etc.) stop re-issuing N+1s when rendering account rows on /accounts. - accounts/_account_sidebar_tabs.html.erb: wrap the partial in a `cache` block keyed on the family's data-version, the current user, shares fingerprint, locale, mobile flag, active tab, and a path-derived "current account" component (`sidebar_active_account_id` helper). The sidebar is rendered on every page in the layout (twice — mobile + desktop drawers), so most navigations now serve the cached fragment instead of re-walking accounts/balances. Local impact (DZG family, 23 accounts, 6.1k transactions): - Dashboard `/`: ~6.5s → ~1.95s - /accounts: ~2.7s → ~0.85s on warm cache - /accountable_sparklines/*: per-request N+1s eliminated; remaining cost is request boilerplate which can be addressed by bumping `RAILS_MAX_THREADS` (the dashboard fans out 5 sparkline turbo frames in parallel and Puma's default 3 threads serialize them). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(perf): address PR review on sidebar/sparkline perf changes - AccountableSparklinesController#requires_normalized_aggregation? also matches legacy plaid_account_id / simplefin_account_id links, not just new-style account_providers, so investment/crypto accounts in the legacy linking state still get LinkedInvestmentSeriesNormalizer applied (Codex P1 / CodeRabbit major). - Sidebar share fingerprint includes both `count` and `max(updated_at)` so deleting a non-most-recent AccountShare invalidates the cached fragment for users who lost access (Codex P1). - Move the sidebar cache-key construction (incl. the AccountShare query) from the ERB into a new `account_sidebar_tabs_cache_key` helper, per the project's "no heavy logic in ERB" rule (CodeRabbit). Co-Authored-By: Claude Opus 4.7 (1M context) * refactor(perf): address human review on perf PR - Account.linked: new SQL-level scope mirroring `Account#linked?` so the controller and per-instance method share one definition. Removes the duplicated raw SQL string in `AccountableSparklinesController#requires_normalized_aggregation?`, which now reads `accounts.linked.exists?` (jjmata, sure-design). - AccountsHelper: move `sidebar_active_account_id` and `account_sidebar_tabs_cache_key` out of `ApplicationHelper`. The cache-key helper also collapses the AccountShare `count` + `max(updated_at)` fingerprint into a single `pick` query so we don't pay two round-trips on every render (jjmata, sure-design). - test/models/account/linkable_test.rb: pin the `Account.linked` scope against all three link types (account_providers, legacy plaid_account, legacy simplefin_account) so any future schema change that diverges the SQL definition from `linked?` breaks a test instead of silently serving wrong sparkline aggregations (sure-design). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(perf): correct shares cache fingerprint on raw-SQL pick `pick(Arel.sql("count(*), max(updated_at)"))` passes a single comma- separated fragment, which Rails returns as a String (per the documented behavior of `pluck` with SQL fragments). The previous `max_at&.to_i` silently truncated `"2025-05-06 12:34:56.789 UTC"` to `2025`, so the sidebar cache key would not change for share `updated_at` movements within the same calendar year — including share deletions — leaving revoked users with a stale sidebar until the 12h expiry. Pass the aggregates as two separate `Arel.sql` args and just concatenate the raw String values into the cache key. The values only need to be stable for a given DB state, not numerically meaningful. Caught by CodeRabbit on PR #1683. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../accountable_sparklines_controller.rb | 8 +++- app/controllers/accounts_controller.rb | 1 + app/helpers/accounts_helper.rb | 37 +++++++++++++++++ app/models/account/linkable.rb | 11 +++++ app/models/balance_sheet/account_totals.rb | 9 +++- .../accounts/_account_sidebar_tabs.html.erb | 2 + app/views/accounts/index/_account_groups.erb | 7 +++- test/models/account/linkable_test.rb | 41 +++++++++++++++++++ 8 files changed, 113 insertions(+), 3 deletions(-) diff --git a/app/controllers/accountable_sparklines_controller.rb b/app/controllers/accountable_sparklines_controller.rb index 9ba69ff56..e32500773 100644 --- a/app/controllers/accountable_sparklines_controller.rb +++ b/app/controllers/accountable_sparklines_controller.rb @@ -43,8 +43,14 @@ class AccountableSparklinesController < ApplicationController ).balance_series end + # balance_type is derived purely from accountable_type, so only Investment/Crypto + # can yield :investment. Short-circuit to avoid an N+1 `account.linked?` check + # on every account for non-investment accountable types (loan, credit_card, etc). + # The `Account.linked` scope is the SQL-level mirror of `Account#linked?`. def requires_normalized_aggregation? - accounts.any? { |account| account.linked? && account.balance_type == :investment } + return false unless %w[Investment Crypto].include?(@accountable.name) + + accounts.linked.exists? end def aggregate_normalized_series diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index f7a4510be..efe9bec08 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -10,6 +10,7 @@ class AccountsController < ApplicationController @manual_accounts = family.accounts .listable_manual .where(id: @accessible_account_ids) + .includes(:accountable, :account_providers, :plaid_account, :simplefin_account) .order(:name) @plaid_items = visible_provider_items(family.plaid_items.ordered.includes(:syncs, :plaid_accounts)) @simplefin_items = visible_provider_items(family.simplefin_items.ordered.includes(:syncs)) diff --git a/app/helpers/accounts_helper.rb b/app/helpers/accounts_helper.rb index 99ef891ce..a119246fd 100644 --- a/app/helpers/accounts_helper.rb +++ b/app/helpers/accounts_helper.rb @@ -8,4 +8,41 @@ module AccountsHelper # Always use the account sync path, which handles syncing all providers sync_account_path(account) end + + # Returns the account id segment from `/accounts/(/...)?`, or nil. + # Used as a cache-key component so the sidebar's active-link styling is + # correct without busting the cache for every unrelated path change. + def sidebar_active_account_id + match = request.path.match(%r{\A/accounts/([\w-]+)}) + match && match[1] + end + + # Cache key for `accounts/_account_sidebar_tabs.html.erb`. + # Kept here (not in the ERB) so the partial stays render-only. + # + # `shares_version` includes both row count and `max(updated_at)` because + # deleting a non-most-recent share would not move `max(updated_at)` and + # could otherwise serve stale fragments to a user who lost access. + # Both are pulled in a single SQL round-trip via `pick`. Note: Rails + # returns the values as Strings for raw SQL fragments — that's fine + # since they only feed into a cache key (concat-stable, never coerced). + def account_sidebar_tabs_cache_key(family:, active_tab:, mobile:) + shares_version = + if Current.user + count, max_at = AccountShare + .where(user_id: Current.user.id) + .pick(Arel.sql("count(*)"), Arel.sql("max(updated_at)")) + "#{count}-#{max_at}" + end + + [ + family.build_cache_key("account_sidebar_tabs_v1", invalidate_on_data_updates: true), + Current.user&.id, + shares_version, + active_tab, + mobile, + I18n.locale, + sidebar_active_account_id + ] + end end diff --git a/app/models/account/linkable.rb b/app/models/account/linkable.rb index d3830ddda..b91d4ed83 100644 --- a/app/models/account/linkable.rb +++ b/app/models/account/linkable.rb @@ -8,6 +8,17 @@ module Account::Linkable # Legacy provider associations - kept for backward compatibility during migration belongs_to :plaid_account, optional: true belongs_to :simplefin_account, optional: true + + # SQL-level mirror of `linked?`. Use this for set-based checks (e.g. bulk + # `EXISTS`) so both definitions stay in sync. If `linked?` adds a new + # provider source, update this scope too. + scope :linked, -> { + left_outer_joins(:account_providers) + .where( + "account_providers.id IS NOT NULL OR accounts.plaid_account_id IS NOT NULL OR accounts.simplefin_account_id IS NOT NULL" + ) + .distinct + } end # A "linked" account gets transaction and balance data from a third party like Plaid or SimpleFin diff --git a/app/models/balance_sheet/account_totals.rb b/app/models/balance_sheet/account_totals.rb index d03340423..5c767687f 100644 --- a/app/models/balance_sheet/account_totals.rb +++ b/app/models/balance_sheet/account_totals.rb @@ -27,7 +27,14 @@ class BalanceSheet::AccountTotals def visible_accounts @visible_accounts ||= begin - scope = family.accounts.visible.with_attached_logo.includes(:account_shares) + scope = family.accounts.visible.with_attached_logo + .includes( + :account_shares, + :accountable, + :plaid_account, + :simplefin_account, + account_providers: :provider + ) scope = scope.accessible_by(user) if user scope end diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index ae6f793d8..653a64c85 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -1,5 +1,6 @@ <%# locals: (family:, active_tab:, mobile: false) %> +<% cache account_sidebar_tabs_cache_key(family: family, active_tab: active_tab, mobile: mobile), expires_in: 12.hours do %>
<% if family.missing_data_provider? %>
@@ -88,3 +89,4 @@ <% end %> <% end %>
+<% end %> diff --git a/app/views/accounts/index/_account_groups.erb b/app/views/accounts/index/_account_groups.erb index 05aa2709f..79709142e 100644 --- a/app/views/accounts/index/_account_groups.erb +++ b/app/views/accounts/index/_account_groups.erb @@ -1,6 +1,11 @@ <%# locals: (accounts:) %> -<% ActiveRecord::Associations::Preloader.new(records: accounts, associations: :account_shares).call if accounts.any? %> +<% if accounts.any? %> + <% ActiveRecord::Associations::Preloader.new( + records: accounts, + associations: [ :account_shares, :accountable, :plaid_account, :simplefin_account, { account_providers: :provider } ] + ).call %> +<% end %> <% accounts.group_by(&:accountable_type).sort_by { |group, _| group }.each do |group, accounts| %>
diff --git a/test/models/account/linkable_test.rb b/test/models/account/linkable_test.rb index a02453c39..560cb5649 100644 --- a/test/models/account/linkable_test.rb +++ b/test/models/account/linkable_test.rb @@ -66,4 +66,45 @@ class Account::LinkableTest < ActiveSupport::TestCase assert @account.can_delete_holdings? end + + # The `linked` scope mirrors `linked?` at the SQL level. These tests pin + # all three link types so a future schema or `linked?` change breaks the + # test instead of silently diverging (e.g. wrong sparkline aggregation). + test "linked scope matches accounts linked via account_providers" do + plaid_account = plaid_accounts(:one) + AccountProvider.create!(account: @account, provider: plaid_account) + + assert_includes Account.linked, @account + end + + test "linked scope matches accounts with legacy plaid_account_id" do + plaid_account = plaid_accounts(:one) + @account.update!(plaid_account: plaid_account) + + assert_includes Account.linked, @account + end + + test "linked scope matches accounts with legacy simplefin_account_id" do + simplefin_item = SimplefinItem.create!( + family: @family, + name: "Test SimpleFin", + access_url: "https://example.com/access_token" + ) + simplefin_account = SimplefinAccount.create!( + simplefin_item: simplefin_item, + name: "Test Account", + account_id: "test-acct", + currency: "USD", + account_type: "checking", + current_balance: 0 + ) + @account.update!(simplefin_account: simplefin_account) + + assert_includes Account.linked, @account + end + + test "linked scope excludes manual accounts" do + assert @account.unlinked? + refute_includes Account.linked, @account + end end