fix(simplefin): Fix some querying issues and various rendering bugs

- Also adding some nice looking progress indicators for loading SimpleFIN data

app/controllers/simple_fin_controller.rb

@@ -3,16 +3,11 @@
 class SimpleFinController < ApplicationController
   before_action :set_accountable_type
   before_action :authenticate_user!
-  before_action :set_simple_fin_provider, only: %i[create new]
+  before_action :set_simple_fin_provider, only: %i[create new accounts_list]
-  before_action :require_simple_fin_provider, only: %i[create new]
+  before_action :require_simple_fin_provider, only: %i[create new accounts_list]
 
   def new
-    @simple_fin_accounts = @simple_fin_provider.get_available_accounts(@accountable_type)
+    # This action now simply renders the view, which will trigger the Turbo Frame load.
-    # Filter accounts we already have
-    @simple_fin_accounts = @simple_fin_accounts.filter { |acc| !account_exists(acc) }
-  rescue StandardError => e
-    Rails.logger.error "SimpleFIN: Failed to fetch accounts - #{e.message}"
-    redirect_to root_path, alert: t(".fetch_failed")
   end
 
   ##
@@ -21,6 +16,23 @@ class SimpleFinController < ApplicationController
     Current.family.accounts.find_by(name: acc["name"])
   end
 
+  def accounts_list
+    simple_fin_accounts_data = @simple_fin_provider.get_available_accounts(@accountable_type)
+    # Filter accounts we already have
+    @simple_fin_accounts = simple_fin_accounts_data.filter { |acc| !account_exists(acc) }
+    # Implicitly renders app/views/simple_fin/accounts_list.html.erb
+  rescue Provider::SimpleFin::RateLimitExceededError => e
+    @error_message = t("simple_fin.new.rate_limit_hit")
+    # Implicitly renders app/views/simple_fin/accounts_list.html.erb
+    render :accounts_list # Or just let it render implicitly
+  rescue StandardError => e
+    Rails.logger.error "SimpleFIN: Failed to fetch accounts from accounts_list - #{e.message}"
+    @error_message = t("simple_fin.new.fetch_failed")
+    # Implicitly renders app/views/simple_fin/accounts_list.html.erb
+    render :accounts_list # Or just let it render implicitly
+  end
+
+
   ##
   # Requests all accounts to be re-synced
   def sync
@@ -80,6 +92,8 @@ class SimpleFinController < ApplicationController
     end
 
     redirect_to root_path, notice: t(".accounts_created_success")
+  rescue Provider::SimpleFin::RateLimitExceededError => e
+    redirect_to new_account_path, alert: t(".rate_limit_hit")
   rescue StandardError => e
     Rails.logger.error "SimpleFIN: Failed to create accounts - #{e.message}"
     redirect_to new_simple_fin_path

app/models/assistant/function/get_accounts.rb

@@ -22,7 +22,7 @@ class Assistant::Function::GetAccounts < Assistant::Function
           type: account.accountable_type,
           start_date: account.start_date,
           is_plaid_linked: account.plaid_account_id.present?,
-          is_simple_fin_linked: account.simple_fin_id.present?,
+          is_simple_fin_linked: account.simple_fin_account_id.present?,
           is_active: account.is_active,
           historical_balances: historical_balances(account)
         }

app/models/provider/simple_fin.rb

@@ -107,20 +107,20 @@ class Provider::SimpleFin
       endpoint += "&end-date=#{trans_end_date}"
     end
 
-    # account_info = send_request_to_sf(endpoint)
+    # request_content = send_request_to_sf(endpoint)
-    # accounts = account_info["accounts"]
+    # # TODO: Remove JSON Reading for real requests. Disabled currently due to preventing rate limits.
-    # TODO: Remove JSON Reading for real requests. Disabled currently due to preventing rate limits.
     json_file_path =  Rails.root.join("sample.simple.fin.json")
     accounts = []
     error_messages = []
     if File.exist?(json_file_path)
-      file_content = File.read(json_file_path)
+      request_content = File.read(json_file_path)
-      parsed_json = JSON.parse(file_content)
-      accounts = parsed_json["accounts"] || []
-      error_messages = parsed_json["errors"] || []
     else
       Rails.logger.warn "SimpleFIN: Sample JSON file not found at #{json_file_path}. Returning empty accounts."
     end
+    # Parse our content
+    parsed_json = JSON.parse(request_content)
+    accounts = parsed_json["accounts"] || []
+    error_messages = parsed_json["errors"] || []
 
 
     # The only way we can really determine types right now is by some properties. Try and set their types

app/models/simple_fin_account.rb

@@ -35,40 +35,53 @@ class SimpleFinAccount < ApplicationRecord
       sfc.simple_fin_accounts.find_or_create_by!(external_id: sf_account_data["id"]) do |sfa|
         balance = get_adjusted_balance(sf_account_data)
         sfa.current_balance = balance
-        sfa.available_balance = balance
+        sfa.available_balance = sf_account_data["available-balance"]&.to_d
         sfa.currency = sf_account_data["currency"]
 
-        new_account = sfc.family.accounts.new(
+
+        if sfa.account
+          account = sfa.account
+        else
+          sfa.account = sfc.family.accounts.new(
             name: sf_account_data["name"],
-          balance: 0,
+            balance: sfa.current_balance,
             currency: sf_account_data["currency"],
             accountable: TYPE_MAPPING[sf_account_data["type"]].new,
             subtype: sf_account_data["subtype"],
             simple_fin_account: sfa, # Explicitly associate back
             last_synced_at: Time.current, # Mark as synced upon creation
             # Set cash_balance similar to how Account.create_and_sync might
-          cash_balance: 0
+            cash_balance: sfa.available_balance
           )
+          account = sfa.account
+          account.save!
 
-        new_account.entries.build(
+          transaction do
+            # Create 2 valuations for new accounts to establish a value history for users to see
+            account.entries.build(
               name: "Current Balance",
               date: Date.current,
-          amount: balance,
+              amount: sfa.current_balance,
-          currency: new_account.currency,
+              currency: account.currency,
               entryable: Valuation.new
             )
-        new_account.entries.build(
+            account.entries.build(
-          name: "Initial Balance", # This will be the balance as of "yesterday"
+              name: "Initial Balance",
               date: 1.day.ago.to_date,
               amount: 0,
-          currency: new_account.currency,
+              currency: account.currency,
               entryable: Valuation.new
             )
 
+            account.save!
+          end
+        end
+
+        # Make sure SFA is up to date
         sfa.save!
-        new_account.save!
+        sfa.sync_account_data!(sf_account_data)
-        new_account.sync_later
+        # Sync this account to trick it into showing a correct current balance
-        sfa.account = new_account
+        account.sync_later
       end
     end
 
@@ -81,18 +94,15 @@ class SimpleFinAccount < ApplicationRecord
   ##
   # Syncs all account data for the given sf_account_data parameter
   def sync_account_data!(sf_account_data)
-    accountable_attributes = { id: self.account.accountable_id }
     balance = SimpleFinAccount.get_adjusted_balance(sf_account_data)
+    puts "SFA #{sf_account_data} #{self.account.inspect}"
     self.update!(
       current_balance: balance,
-      available_balance: sf_account_data["available-balance"]&.to_d,
+      available_balance: sf_account_data["available-balance"]&.to_d
-      currency: sf_account_data["currency"],
+    )
-      account_attributes: {
+
-        id: self.account.id,
+    self.account.update!(
-        balance: balance,
+      balance: balance
-        last_synced_at: Time.current,
-        accountable_attributes: accountable_attributes
-      }
     )
 
     institution_errors = sf_account_data["org"]["institution_errors"]
@@ -107,7 +117,7 @@ class SimpleFinAccount < ApplicationRecord
       sync_transactions!(sf_account_data["transactions"])
     end
 
-    # Sync holdings if present in the data and it's an investment account
+    # Sync holdings if present in the data and it's an investment account. SimpleFIN doesn't support transactions for holdings accounts
     if self.account&.investment? && sf_account_data["holdings"].is_a?(Array)
       sync_holdings!(sf_account_data["holdings"])
     end

app/views/accounts/_account_error.html.erb

@@ -1,7 +1,7 @@
 <%# locals: (account:, link_to_path: nil, given_title: nil, target: nil) %>
 
 <%# Flag indicators of account issues %>
-<% if account.simple_fin_account.simple_fin_item.status == "requires_update" %>
+<% if account.simple_fin_account&.simple_fin_item&.status == "requires_update" %>
     <%= link_to link_to_path, target: target do %>
         <%= icon(
             "alert-triangle",

app/views/accounts/new.html.erb

@@ -1,4 +1,11 @@
 <%= render layout: "accounts/new/container", locals: { title: t(".title") } do %>
+
+<%# Display flash messages, especially alerts for errors %>
+  <% if flash[:alert] %>
+    <div class="mb-4 p-4 text-sm text-red-700 bg-red-100 rounded-lg border border-red-300" role="alert">
+      <%= flash[:alert] %>
+    </div>
+  <% end %>
   <div class="text-sm">
     <% unless params[:classification] == "liability" %>
       <%= render "account_type", accountable: Depository.new %>

app/views/accounts/show/_activity.html.erb

@@ -4,7 +4,7 @@
   <div class="bg-container p-5 shadow-border-xs rounded-xl" data-controller="focus-record" data-focus-record-id-value="<%= @focused_record ? dom_id(@focused_record) : nil %>">
     <div class="flex items-center justify-between mb-4" data-testid="activity-menu">
       <%= tag.h2 t(".title"), class: "font-medium text-lg" %>
-      <% unless (@account.plaid_account_id.present? || @account.simple_fin_account.present?) %>
+      <% unless (@account.plaid_account_id.present? || @account.simple_fin_account_id.present?) %>
         <%= render MenuComponent.new(variant: "button") do |menu| %>
           <% menu.with_button(text: "New", variant: "secondary", icon: "plus") %>
 

app/views/accounts/show/_header.html.erb

@@ -36,7 +36,8 @@
             size: "sm",
             href: sync_plaid_item_path(account.plaid_account.plaid_item),
             disabled: account.syncing?,
-            frame: :_top
+            frame: :_top,
+            title: "Refresh all SimpleFIN accounts"
           ) %>
         <% end %>
       <% elsif account.simple_fin_account_id.present? %>

app/views/holdings/index.html.erb

@@ -1,5 +1,6 @@
 <%= turbo_frame_tag dom_id(@account, "holdings") do %>
   <div class="bg-container space-y-4 p-5 rounded-xl shadow-border-xs">
+    <% unless (@account.plaid_account_id.present? || @account.simple_fin_account_id.present?) %>
       <div class="flex items-center justify-between">
         <%= tag.h2 t(".holdings"), class: "font-medium text-lg" %>
         <%= link_to new_trade_path(account_id: @account.id),
@@ -12,6 +13,7 @@
           <%= tag.span t(".new_holding"), class: "text-sm" %>
         <% end %>
       </div>
+    <% end %>
 
     <div class="rounded-xl bg-container-inset p-1">
       <div class="grid grid-cols-12 items-center uppercase text-xs font-medium text-secondary px-4 py-2">

app/views/holdings/show.html.erb

@@ -79,7 +79,7 @@
       </div>
     <% end %>
 
-    <% unless @holding.account.plaid_account_id.present? %>
+    <% unless (@holding.account.plaid_account_id.present? || @holding.account.simple_fin_account_id.present? ) %>
       <% dialog.with_section(title: t(".settings"), open: true) do %>
         <div class="pb-4">
           <div class="flex items-center justify-between gap-2 p-3">

app/views/simple_fin/_accounts_list_content.html.erb

@@ -0,0 +1,65 @@
+<%# locals: simple_fin_accounts, accountable_type %>
+
+<% if simple_fin_accounts.blank? %>
+  <div class="text-center text-gray-500 py-8">
+    <p>No accounts found matching this type from SimpleFIN.</p>
+    <p>Please ensure your SimpleFIN subscription is active.</p>
+  </div>
+<% else %>
+  <%= styled_form_with url: simple_fin_index_path(accountable_type: accountable_type), method: :post, data: { turbo: false } do |form| %>
+    <%# Render each account option parsed from SimpleFIN %>
+    <% simple_fin_accounts.each_with_index do |account, index| %>
+      <div class="mb-4">
+        <label class="flex items-start justify-between p-4 hover:bg-surface rounded-lg border border-gray-200 cursor-pointer">
+          <div class="flex items-center gap-3">
+            <%= check_box_tag 'selected_account_ids[]', account["id"], class: "border-gray-300 rounded" -%>
+            <div class="flex-grow">
+              <%# Account Name %>
+              <div class="font-medium text-primary"><%= account["name"] %></div>
+              <%# Account Source %>
+              <% if account.dig("org", "name").present? %>
+                <div class="text-xs text-gray-400 mt-0.5">
+                  Source:
+                  <% if account.dig("org", "url").present? %>
+                    <%= link_to account.dig("org", "name"), account.dig("org", "url"), target: "_blank", rel: "noopener noreferrer", class: "hover:underline" %>
+                  <% else %>
+                    <%= account.dig("org", "name") %>
+                  <% end %>
+                </div>
+              <% end %>
+              
+              <%# SimpleFIN has a tough time determining account types. Let the user fill them out here. %>
+              <div class="mt-2">
+                <% if accountable_type == "Depository" %>
+                  <%= form.select "account[#{account['id']}][subtype]",
+                        Depository::SUBTYPES.map { |k, v| [v[:long], k] },
+                        { label: "Subtype", prompt: t("depositories.form.subtype_prompt"), include_blank: t("depositories.form.none") } %>
+                <% elsif accountable_type == "Investment" %>
+                  <%= form.select "account[#{account['id']}][subtype]",
+                        Investment::SUBTYPES.map { |k, v| [v[:long], k] },
+                        { label: "Subtype", prompt: t("investments.form.subtype_prompt"), include_blank: t("investments.form.none") } %>
+                <% end %>
+              </div>
+            </div>
+          </div>
+
+          <%# Current balance %>
+          <% currency_unit = Money::Currency.all_instances.find { |currency| currency.iso_code == account["currency"] }.symbol %>
+          <% balance_color_class = account["balance"].to_f < 0 ? "text-red-500" : "text-green-500" %>
+          <div class="text-sm text-gray-700 font-medium">
+            <span class="<%= balance_color_class %>">
+              <%= number_to_currency(account["balance"], unit: currency_unit || "$") %>
+            </span>
+          </div>
+        </label>
+      </div>
+    <% end %>
+
+    <div class="mt-6 pt-6 border-t border-gray-200">
+      <%= form.submit t("simple_fin.form.add_accounts"), class: "w-full btn btn-primary",
+        data: {
+          disable_with: "Adding Accounts..."
+        } %>
+    </div>
+  <% end %>
+<% end %>

app/views/simple_fin/_error_display.html.erb

@@ -0,0 +1,4 @@
+<%# locals: message, accountable_type %>
+<div class="text-center text-red-500 py-8">
+  <p><%= message %></p>
+</div>

app/views/simple_fin/accounts_list.html.erb

@@ -0,0 +1,7 @@
+<turbo-frame id="simple_fin_accounts_data">
+  <% if @error_message %>
+    <%= render partial: "simple_fin/error_display", locals: { message: @error_message, accountable_type: @accountable_type } %>
+  <% else %>
+    <%= render partial: "simple_fin/accounts_list_content", locals: { simple_fin_accounts: @simple_fin_accounts, accountable_type: @accountable_type } %>
+  <% end %>
+</turbo-frame>

app/views/simple_fin/new.html.erb

@@ -1,70 +1,14 @@
 <%# locals: @simple_fin_accounts %>
 
 <%= render layout: "accounts/new/container", locals: { title: "Select SimpleFIN Accounts", back_path: new_account_path } do %>
-  <% if @simple_fin_accounts.blank? %>
+  <turbo-frame id="simple_fin_accounts_data" src="<%= simple_fin_accounts_list_path(accountable_type: @accountable_type) %>">
     <div class="text-center text-gray-500 py-8">
-      <p>No accounts found matching this type from SimpleFIN.</p>
+      <p>Loading accounts...</p>
-      <p>Please ensure your SimpleFIN subscription is active.</p>
+      <%# Basic SVG Spinner (Tailwind CSS for animation) %>
-      <%= link_to "Try Again", new_simple_fin_path(accountable_type: @accountable_type), class: "mt-4 inline-block text-primary hover:underline" %>
+      <svg class="animate-spin h-8 w-8 text-primary mx-auto mt-4" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24">
+        <circle class="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" stroke-width="4"></circle>
+        <path class="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z"></path>
+      </svg>
     </div>
-  <% else %>
+  </turbo-frame>
-    <%= styled_form_with url: simple_fin_index_path(accountable_type: @accountable_type), method: :post, data: { turbo: false } do |form| %>
-      <%# Render each account option parsed from SimpleFIN %>
-      <% @simple_fin_accounts.each_with_index do |account, index| %>
-        <div class="mb-4">
-          <label class="flex items-start justify-between p-4 hover:bg-surface rounded-lg border border-gray-200 cursor-pointer">
-            <div class="flex items-center gap-3">
-              <%= check_box_tag 'selected_account_ids[]', account["id"], class: "border-gray-300 rounded" -%>
-              <div class="flex-grow">
-                <%# Account Name %>
-                <div class="font-medium text-primary"><%= account["name"] %></div>
-                <%# Account Source %>
-                <% if account.dig("org", "name").present? %>
-                  <div class="text-xs text-gray-400 mt-0.5">
-                    Source:
-                    <% if account.dig("org", "url").present? %>
-                      <%= link_to account.dig("org", "name"), account.dig("org", "url"), target: "_blank", rel: "noopener noreferrer", class: "hover:underline" %>
-                    <% else %>
-                      <%= account.dig("org", "name") %>
-                    <% end %>
-                  </div>
-                <% end %>
-                
-                <%# SimpleFIN has a tough time determining account types. Let the user fill them out here. %>
-                <div class="mt-2">
-                  <% if @accountable_type == "Depository" %>
-                    <%= form.select "account[#{account['id']}][subtype]",
-                          Depository::SUBTYPES.map { |k, v| [v[:long], k] },
-                          { label: "Subtype", prompt: t("depositories.form.subtype_prompt"), include_blank: t("depositories.form.none") } %>
-                  <% elsif @accountable_type == "Investment" %>
-                    <%= form.select "account[#{account['id']}][subtype]",
-                          Investment::SUBTYPES.map { |k, v| [v[:long], k] },
-                          { label: "Subtype", prompt: t("investments.form.subtype_prompt"), include_blank: t("investments.form.none") } %>
-                  <% end %>
-                </div>
-                
-              </div>
-            </div>
-
-            <%# Current balance %>
-            <% currency_unit = Money::Currency.all_instances.find { |currency| currency.iso_code == account["currency"] }.symbol %>
-            <% balance_color_class = account["balance"].to_f < 0 ? "text-red-500" : "text-green-500" %>
-            <div class="text-sm text-gray-700 font-medium">
-              <span class="<%= balance_color_class %>">
-                <%= number_to_currency(account["balance"], unit: currency_unit || "$") %>
-              </span>
-            </div>
-          </label>
-        </div>
-      <% end %>
-
-
-      <div class="mt-6 pt-6 border-t border-gray-200">
-        <%= form.submit t("simple_fin.form.add_accounts"), class: "w-full btn btn-primary",
-          data: {
-            disable_with: "Adding Accounts..."
-          } %>
-      </div>
-    <% end %>
-  <% end %>
 <% end %>

app/views/simple_fin_items/_simple_fin_item.html.erb

@@ -1,20 +1,15 @@
 <%# locals: (simple_fin_item:) %>
 
-<div class="group bg-container p-4 shadow-border-xs rounded-xl">
+<% if simple_fin_item.accounts.any? %>
+  <div class="group bg-container p-4 shadow-border-xs rounded-xl">
     <div class="px-4 py-3 sm:px-6 border-b border-gray-200">
       <div class="flex items-center justify-between">
         <div class="flex items-center gap-3">
-        <% if simple_fin_item.logo.attached? %>
-          <%= image_tag simple_fin_item.logo, class: "h-8 w-8 rounded-full" %>
-        <% else %>
-          <div>
-            <%= simple_fin_item.institution_name&.first || "?" %>
-          </div>
-        <% end %>
           <h3 >
             <%= simple_fin_item.institution_name || "SimpleFIN Connection" %>
           </h3>
         </div>
+        <%= image_tag "simple-fin-logo.svg", class: "h-6 w-auto", title: "Connected via SimpleFIN" %>
       </div>
     </div>
 
@@ -23,4 +18,5 @@
         <%= render "accounts/account", account: account %>
       <% end %>
     </ul>
-</div>
+  </div>
+<% end %>

config/locales/simple_fin/en.yml

@@ -5,7 +5,9 @@ en:
       add_accounts: "Add Selected Accounts"
     new:
       fetch_failed: "Failed to fetch accounts"
+      rate_limit_hit: "You've exceeded the SimpleFIN Rate limit for today"
     create:
       accounts_created_success: "Accounts Successfully Created"
       fetch_failed: "Failed to fetch accounts"
       no_acc_selected: "No accounts were selected to add"
+      rate_limit_hit: "You've exceeded the SimpleFIN Rate limit for today"

config/routes.rb

@@ -201,6 +201,7 @@ Rails.application.routes.draw do
   end
 
   # SimpleFIN routes
+  get "/simple_fin/accounts_list", to: "simple_fin#accounts_list", as: :simple_fin_accounts_list
   resources :simple_fin, only: %i[create new] do
     member do
       post :sync