Consolidate and simplify account pages (#2462)

* Remove ScrollFocusable

* Consolidate and simplify account pages

* Lint fixes

* Fix tab param initialization

* Remove stale files

* Remove stale route, make accountable routes clearer

Author: zachgoll

app/views/accounts/show/_chart.html.erb renamed to app/components/UI/account/chart.html.erb

@@ -1,32 +1,28 @@
-<%# locals: (account:, tooltip: nil, chart_view: nil, **args) %>
-
-<% period =  @period || Period.last_30_days %>
-<% default_value_title = account.asset? ? t(".balance") : t(".owed") %>
-
 <div id="<%= dom_id(account, :chart) %>" class="bg-container shadow-border-xs rounded-xl space-y-2">
   <div class="flex justify-between flex-col-reverse lg:flex-row gap-2 px-4 pt-4 mb-2">
     <div class="space-y-2 w-full">
       <div class="flex items-center gap-1">
-        <%= tag.p account.investment? ? "Total value" : default_value_title, class: "text-sm font-medium text-secondary" %>
+        <%= tag.p title, class: "text-sm font-medium text-secondary" %>
 
         <% if account.investment? %>
-          <%= render "investments/value_tooltip", balance: account.balance_money, holdings: account.balance_money - account.cash_balance_money, cash: account.cash_balance_money %>
+          <%= render "investments/value_tooltip", balance: account.balance_money, holdings: holdings_value_money, cash: account.cash_balance_money %>
         <% end %>
       </div>
       <div class="flex flex-row gap-2 items-baseline">
-        <%= tag.p format_money(account.balance_money), class: "text-primary text-3xl font-medium truncate" %>
-        <% if account.currency != Current.family.currency %>
-          <%= tag.p format_money(account.balance_money.exchange_to(Current.family.currency, fallback_rate: 1)), class: "text-sm font-medium text-secondary" %>
+        <%= tag.p view_balance_money.format, class: "text-primary text-3xl font-medium truncate" %>
+
+        <% if converted_balance_money %>
+          <%= tag.p converted_balance_money.format, class: "text-sm font-medium text-secondary" %>
         <% end %>
       </div>
     </div>
 
-    <%= form_with url: request.path, method: :get, data: { controller: "auto-submit-form" } do |form| %>
+    <%= form_with url: account_path(account), method: :get, data: { controller: "auto-submit-form" } do |form| %>
       <div class="flex items-center gap-2">
-        <% if chart_view.present? %>
+        <% if account.investment? %>
           <%= form.select :chart_view,
             [["Total value", "balance"], ["Holdings", "holdings_balance"], ["Cash", "cash_balance"]],
-            { selected: chart_view },
+            { selected: view },
             class: "bg-container border border-secondary rounded-lg text-sm pr-7 cursor-pointer text-primary focus:outline-hidden focus:ring-0",
             data: { "auto-submit-form-target": "auto" } %>
         <% end %>
@@ -40,7 +36,23 @@
     <% end %>
   </div>
 
-  <%= turbo_frame_tag dom_id(account, :chart_details), src: chart_account_path(account, period: period.key, chart_view: chart_view) do %>
-    <%= render "accounts/chart_loader" %>
+  <%= turbo_frame_tag dom_id(@account, :chart_details) do %>
+    <div class="px-4">
+      <%= render partial: "shared/trend_change", locals: { trend: trend, comparison_label: period.comparison_label } %>
+    </div>
+
+    <div class="h-64 pb-4">
+      <% if series.any? %>
+        <div
+        id="lineChart"
+        class="w-full h-full"
+        data-controller="time-series-chart"
+        data-time-series-chart-data-value="<%= series.to_json %>"></div>
+      <% else %>
+        <div class="w-full h-full flex items-center justify-center">
+          <p class="text-secondary text-sm">No data available</p>
+        </div>
+      <% end %>
+    </div>
   <% end %>
 </div>

app/components/UI/account/chart.rb

@@ -0,0 +1,72 @@
+class UI::Account::Chart < ApplicationComponent
+  attr_reader :account
+
+  def initialize(account:, period: nil, view: nil)
+    @account = account
+    @period = period
+    @view = view
+  end
+
+  def period
+    @period ||= Period.last_30_days
+  end
+
+  def holdings_value_money
+    account.balance_money - account.cash_balance_money
+  end
+
+  def view_balance_money
+    case view
+    when "balance"
+      account.balance_money
+    when "holdings_balance"
+      holdings_value_money
+    when "cash_balance"
+      account.cash_balance_money
+    end
+  end
+
+  def title
+    case account.accountable_type
+    when "Investment", "Crypto"
+      case view
+      when "balance"
+        "Total account value"
+      when "holdings_balance"
+        "Holdings value"
+      when "cash_balance"
+        "Cash value"
+      end
+    when "Property", "Vehicle"
+      "Estimated #{account.accountable_type.humanize.downcase} value"
+    when "CreditCard", "OtherLiability"
+      "Debt balance"
+    when "Loan"
+      "Remaining principal balance"
+    else
+      "Balance"
+    end
+  end
+
+  def foreign_currency?
+    account.currency != account.family.currency
+  end
+
+  def converted_balance_money
+    return nil unless foreign_currency?
+
+    account.balance_money.exchange_to(account.family.currency, fallback_rate: 1)
+  end
+
+  def view
+    @view ||= "balance"
+  end
+
+  def series
+    account.balance_series(period: period, view: view)
+  end
+
+  def trend
+    series.trend
+  end
+end

app/components/UI/account_page.html.erb

@@ -0,0 +1,29 @@
+<%= turbo_stream_from account %>
+
+<%= turbo_frame_tag dom_id(account, :container) do %>
+  <%= tag.div class: "space-y-4 pb-32" do %>
+    <%= render "accounts/show/header", account: account, title: title, subtitle: subtitle %>
+
+    <%= render UI::Account::Chart.new(account: account, period: chart_period, view: chart_view) %>
+
+    <div class="min-h-[800px]" data-testid="account-details">
+      <% if tabs.count > 1 %>
+        <%= render TabsComponent.new(active_tab: active_tab, url_param_key: "tab") do |tabs_container| %>
+          <% tabs_container.with_nav(classes: "max-w-fit") do |nav| %>
+            <% tabs.each do |tab| %>
+              <% nav.with_btn(id: tab, label: tab.to_s.humanize, classes: "px-6") %>
+            <% end %>
+          <% end %>
+
+          <% tabs.each do |tab| %>
+            <% tabs_container.with_panel(tab_id: tab) do %>
+              <%= render tab_partial_name(tab), account: account %>
+            <% end %>
+          <% end %>
+        <% end %>
+      <% else %>
+        <%= render tab_partial_name(tabs.first), account: account %>
+      <% end %>
+    </div>
+  <% end %>
+<% end %>

app/components/UI/account_page.rb

@@ -0,0 +1,45 @@
+class UI::AccountPage < ApplicationComponent
+  attr_reader :account, :chart_view, :chart_period
+
+  def initialize(account:, chart_view: nil, chart_period: nil, active_tab: nil)
+    @account = account
+    @chart_view = chart_view
+    @chart_period = chart_period
+    @active_tab = active_tab
+  end
+
+  def title
+    account.name
+  end
+
+  def subtitle
+    return nil unless account.property?
+
+    account.property.address
+  end
+
+  def active_tab
+    tabs.find { |tab| tab == @active_tab&.to_sym } || tabs.first
+  end
+
+  def tabs
+    case account.accountable_type
+    when "Investment"
+      [ :activity, :holdings ]
+    when "Property", "Vehicle", "Loan"
+      [ :activity, :overview ]
+    else
+      [ :activity ]
+    end
+  end
+
+  def tab_partial_name(tab)
+    case tab
+    when :activity
+      "accounts/show/activity"
+    when :holdings, :overview
+      # Accountable is responsible for implementing the partial in the correct folder
+      "#{account.accountable_type.downcase.pluralize}/tabs/#{tab}"
+    end
+  end
+end

app/components/application_component.rb

@@ -0,0 +1,4 @@
+class ApplicationComponent < ViewComponent::Base
+  # These don't work as expected with helpers.turbo_frame_tag, etc., so we include them here
+  include Turbo::FramesHelper, Turbo::StreamsHelper
+end

app/controllers/accounts_controller.rb

@@ -1,5 +1,5 @@
 class AccountsController < ApplicationController
-  before_action :set_account, only: %i[sync chart sparkline toggle_active]
+  before_action :set_account, only: %i[sync sparkline toggle_active show destroy]
   include Periodable
 
   def index
@@ -9,6 +9,15 @@ def index
     render layout: "settings"
   end
 
+  def show
+    @chart_view = params[:chart_view] || "balance"
+    @tab = params[:tab]
+    @q = params.fetch(:q, {}).permit(:search)
+    entries = @account.entries.search(@q).reverse_chronological
+
+    @pagy, @entries = pagy(entries, limit: params[:per_page] || "10")
+  end
+
   def sync
     unless @account.syncing?
       @account.sync_later
@@ -17,11 +26,6 @@ def sync
     redirect_to account_path(@account)
   end
 
-  def chart
-    @chart_view = params[:chart_view] || "balance"
-    render layout: "application"
-  end
-
   def sparkline
     etag_key = @account.family.build_cache_key("#{@account.id}_sparkline", invalidate_on_data_updates: true)
 
@@ -42,6 +46,15 @@ def toggle_active
     redirect_to accounts_path
   end
 
+  def destroy
+    if @account.linked?
+      redirect_to account_path(@account), alert: "Cannot delete a linked account"
+    else
+      @account.destroy_later
+      redirect_to accounts_path, notice: "Account scheduled for deletion"
+    end
+  end
+
   private
     def family
       Current.family

app/controllers/concerns/accountable_resource.rb

@@ -2,9 +2,9 @@ module AccountableResource
   extend ActiveSupport::Concern
 
   included do
-    include ScrollFocusable, Periodable
+    include Periodable
 
-    before_action :set_account, only: [ :show, :edit, :update, :destroy ]
+    before_action :set_account, only: [ :show, :edit, :update ]
     before_action :set_link_options, only: :new
   end
 
@@ -27,9 +27,7 @@ def show
     @q = params.fetch(:q, {}).permit(:search)
     entries = @account.entries.search(@q).reverse_chronological
 
-    set_focused_record(entries, params[:focused_record_id])
-
-    @pagy, @entries = pagy(entries, limit: params[:per_page] || "10", params: ->(params) { params.except(:focused_record_id) })
+    @pagy, @entries = pagy(entries, limit: params[:per_page] || "10")
   end
 
   def edit
@@ -63,16 +61,7 @@ def update
     end
 
     @account.lock_saved_attributes!
-    redirect_back_or_to @account, notice: t("accounts.update.success", type: accountable_type.name.underscore.humanize)
-  end
-
-  def destroy
-    if @account.linked?
-      redirect_to account_path(@account), alert: "Cannot delete a linked account"
-    else
-      @account.destroy_later
-      redirect_to accounts_path, notice: t("accounts.destroy.success", type: accountable_type.name.underscore.humanize)
-    end
+    redirect_back_or_to account_path(@account), notice: t("accounts.update.success", type: accountable_type.name.underscore.humanize)
   end
 
   private

app/controllers/concerns/scroll_focusable.rb

@@ -1,5 +1,5 @@
 class TransactionsController < ApplicationController
-  include ScrollFocusable, EntryableResource
+  include EntryableResource
 
   before_action :store_params!, only: :index
 
@@ -21,12 +21,7 @@ def index
                          :transfer_as_inflow, :transfer_as_outflow
                        )
 
-    @pagy, @transactions = pagy(base_scope, limit: per_page, params: ->(p) { p.except(:focused_record_id) })
-
-    # No performance penalty by default. Only runs queries if the record is set.
-    if params[:focused_record_id].present?
-      set_focused_record(base_scope, params[:focused_record_id], default_per_page: per_page)
-    end
+    @pagy, @transactions = pagy(base_scope, limit: per_page)
   end
 
   def clear_filter