perf(accounts): kill sidebar/sparkline N+1s and cache the sidebar (#1683)

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

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))

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/<id>(/...)?`, 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

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

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

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 %>
 <div id="account-sidebar-tabs">
   <% if family.missing_data_provider? %>
     <details class="group bg-yellow-tint-10 rounded-lg p-2 text-yellow-600 mb-3 text-xs">
@@ -88,3 +89,4 @@
     <% end %>
   <% end %>
 </div>
+<% end %>

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| %>
   <div class="bg-container-inset p-1 rounded-xl">
     <div class="flex items-center px-4 py-2 text-xs font-medium text-secondary">

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