Remove SimpleFIN sync errors modal and related routes, methods, and logic. (#365)

- Removed the `errors` modal and its associated view.
- Eliminated references to `errors` route and controller methods.
- Consolidated error handling into the `register_error` method to improve error tracking and de-duplication.
- Enhanced logging and introduced instrumentation for better observability.

Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>

app/controllers/simplefin_items_controller.rb

@@ -1,6 +1,6 @@
 class SimplefinItemsController < ApplicationController
   include SimplefinItems::MapsHelper
-  before_action :set_simplefin_item, only: [ :show, :edit, :update, :destroy, :sync, :balances, :setup_accounts, :complete_account_setup, :errors ]
+  before_action :set_simplefin_item, only: [ :show, :edit, :update, :destroy, :sync, :balances, :setup_accounts, :complete_account_setup ]
 
   def index
     @simplefin_items = Current.family.simplefin_items.active.ordered
@@ -366,35 +366,6 @@ class SimplefinItemsController < ApplicationController
     end
   end
 
-  def errors
-    # Find the latest sync to surface its error messages in a lightweight modal
-    latest_sync = if @simplefin_item.syncs.loaded?
-      @simplefin_item.syncs.max_by(&:created_at)
-    else
-      @simplefin_item.syncs.ordered.first
-    end
-
-    stats = (latest_sync&.sync_stats || {})
-    raw_errors = Array(stats["errors"]) # may contain strings or hashes with message keys
-
-    @errors = raw_errors.map { |e|
-      if e.is_a?(Hash)
-        e["message"] || e[:message] || e.to_s
-      else
-        e.to_s
-      end
-    }.compact
-
-    # Fall back to simplefin_item.sync_error if present and not already included
-    if @simplefin_item.respond_to?(:sync_error) && @simplefin_item.sync_error.present?
-      @errors << @simplefin_item.sync_error
-    end
-
-    # De-duplicate and keep non-empty messages
-    @errors = @errors.map(&:to_s).map(&:strip).reject(&:blank?).uniq
-
-    render layout: false
-  end
 
   private
 

app/helpers/simplefin_items_helper.rb

@@ -18,11 +18,15 @@ module SimplefinItemsHelper
     total_errors = stats["total_errors"].to_i
     return nil if total_errors.zero?
 
-    sample = Array(stats["errors"]).map do |e|
+    # Build a small, de-duplicated sample of messages with counts
+    grouped = Array(stats["errors"]).map { |e|
       name = (e[:name] || e["name"]).to_s
       msg  = (e[:message] || e["message"]).to_s
-      name.present? ? "#{name}: #{msg}" : msg
-    end.compact.first(2).join(" • ")
+      text = name.present? ? "#{name}: #{msg}" : msg
+      text.strip
+    }.reject(&:blank?).tally
+
+    sample = grouped.first(2).map { |text, count| count > 1 ? "#{text} (×#{count})" : text }.join(" • ")
 
     buckets = stats["error_buckets"] || {}
     bucket_text = if buckets.present?

app/models/simplefin_item/importer.rb

@@ -1,3 +1,4 @@
+require "set"
 class SimplefinItem::Importer
   class RateLimitedError < StandardError; end
   attr_reader :simplefin_item, :simplefin_provider, :sync
@@ -54,13 +55,8 @@ class SimplefinItem::Importer
         import_account_minimal_and_balance(account_data)
       rescue => e
         stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
-        stats["errors"] ||= []
-        stats["total_errors"] = stats.fetch("total_errors", 0) + 1
         cat = classify_error(e)
-        buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
-        buckets[cat] = buckets.fetch(cat, 0) + 1
-        stats["errors"] << { account_id: account_data[:id], name: account_data[:name], message: e.message.to_s, category: cat }
-        stats["errors"] = stats["errors"].last(5)
+        register_error(message: e.message, category: cat, account_id: account_data[:id], name: account_data[:name])
       ensure
         persist_stats!
       end
@@ -90,15 +86,11 @@ class SimplefinItem::Importer
       rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e
         # Surface a friendly duplicate/validation signal in sync stats and continue
         stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
-        stats["errors"] ||= []
-        stats["total_errors"] = stats.fetch("total_errors", 0) + 1
-        cat = "other"
         msg = e.message.to_s
         if msg.downcase.include?("already been taken") || msg.downcase.include?("unique")
           msg = "Duplicate upstream account detected for SimpleFin (account_id=#{account_id}). Try relinking to an existing manual account."
         end
-        stats["errors"] << { account_id: account_id, name: account_data[:name], message: msg, category: cat }
-        stats["errors"] = stats["errors"].last(5)
+        register_error(message: msg, category: "other", account_id: account_id, name: account_data[:name])
         persist_stats!
         return
       end
@@ -117,6 +109,49 @@ class SimplefinItem::Importer
       @stats ||= {}
     end
 
+    # Track seen error fingerprints during a single importer run to avoid double counting
+    def seen_errors
+      @seen_errors ||= Set.new
+    end
+
+    # Register an error into stats with de-duplication and bucketing
+    def register_error(message:, category:, account_id: nil, name: nil)
+      msg = message.to_s.strip
+      cat = (category.presence || "other").to_s
+      fp = [ account_id.to_s.presence, cat, msg ].compact.join("|")
+      first_time = !seen_errors.include?(fp)
+      seen_errors.add(fp)
+
+      if first_time
+        Rails.logger.warn(
+          "SimpleFin sync error (unique this run): category=#{cat} account_id=#{account_id.inspect} name=#{name.inspect} msg=#{msg}"
+        )
+        # Emit an instrumentation event for observability dashboards
+        ActiveSupport::Notifications.instrument(
+          "simplefin.error",
+          item_id: simplefin_item.id,
+          account_id: account_id,
+          account_name: name,
+          category: cat,
+          message: msg
+        )
+      else
+        # Keep logs tame; don't spam on repeats in the same run
+      end
+
+      stats["errors"] ||= []
+      buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
+      if first_time
+        stats["total_errors"] = stats.fetch("total_errors", 0) + 1
+        buckets[cat] = buckets.fetch(cat, 0) + 1
+      end
+
+      # Maintain a small rolling sample (not de-duped so users can see most recent context)
+      stats["errors"] << { account_id: account_id, name: name, message: msg, category: cat }
+      stats["errors"] = stats["errors"].last(5)
+      persist_stats!
+    end
+
     def persist_stats!
       return unless sync && sync.respond_to?(:sync_stats)
       merged = (sync.sync_stats || {}).merge(stats)
@@ -196,21 +231,9 @@ class SimplefinItem::Importer
           rescue => e
             stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
             # Collect lightweight error info for UI stats
-            stats["errors"] ||= []
-            stats["total_errors"] = stats.fetch("total_errors", 0) + 1
             cat = classify_error(e)
-            buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
-            buckets[cat] = buckets.fetch(cat, 0) + 1
             begin
-              err_item = {
-                account_id: account_data[:id],
-                name: account_data[:name],
-                message: e.message.to_s,
-                category: cat
-              }
-              stats["errors"] << err_item
-              # Keep only a small sample for UI (avoid blowing up sync_stats)
-              stats["errors"] = stats["errors"].last(5)
+              register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name])
             rescue
               # no-op if account_data is missing keys
             end
@@ -255,19 +278,9 @@ class SimplefinItem::Importer
           import_account(account_data)
         rescue => e
           stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
-          stats["errors"] ||= []
-          stats["total_errors"] = stats.fetch("total_errors", 0) + 1
           cat = classify_error(e)
-          buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
-          buckets[cat] = buckets.fetch(cat, 0) + 1
           begin
-            stats["errors"] << {
-              account_id: account_data[:id],
-              name: account_data[:name],
-              message: e.message.to_s,
-              category: cat
-            }
-            stats["errors"] = stats["errors"].last(5)
+            register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name])
           rescue
             # no-op if account_data is missing keys
           end
@@ -308,19 +321,9 @@ class SimplefinItem::Importer
             import_account(account_data)
           rescue => e
             stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
-            stats["errors"] ||= []
-            stats["total_errors"] = stats.fetch("total_errors", 0) + 1
             cat = classify_error(e)
-            buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
-            buckets[cat] = buckets.fetch(cat, 0) + 1
             begin
-              stats["errors"] << {
-                account_id: account_data[:id],
-                name: account_data[:name],
-                message: e.message.to_s,
-                category: cat
-              }
-              stats["errors"] = stats["errors"].last(5)
+              register_error(message: e.message.to_s, category: cat, account_id: account_data[:id], name: account_data[:name])
             rescue
               # no-op if account_data is missing keys
             end
@@ -464,15 +467,11 @@ class SimplefinItem::Importer
       rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e
         # Treat duplicates/validation failures as partial success: count and surface friendly error, then continue
         stats["accounts_skipped"] = stats.fetch("accounts_skipped", 0) + 1
-        stats["errors"] ||= []
-        stats["total_errors"] = stats.fetch("total_errors", 0) + 1
-        cat = "other"
         msg = e.message.to_s
         if msg.downcase.include?("already been taken") || msg.downcase.include?("unique")
           msg = "Duplicate upstream account detected for SimpleFin (account_id=#{account_id}). Try relinking to an existing manual account."
         end
-        stats["errors"] << { account_id: account_id, name: account_data[:name], message: msg, category: cat }
-        stats["errors"] = stats["errors"].last(5)
+        register_error(message: msg, category: "other", account_id: account_id, name: account_data[:name])
         persist_stats!
         nil
       end
@@ -498,11 +497,23 @@ class SimplefinItem::Importer
         end
       end
 
-      simplefin_item.update!(status: :requires_update) if needs_update
+      if needs_update
+        Rails.logger.warn("SimpleFin: marking item ##{simplefin_item.id} requires_update due to auth-related provider errors")
+        simplefin_item.update!(status: :requires_update)
+        ActiveSupport::Notifications.instrument(
+          "simplefin.item_requires_update",
+          item_id: simplefin_item.id,
+          reason: "provider_errors_partial",
+          count: arr.size
+        )
+      end
 
-      stats["errors"] ||= []
-      stats["total_errors"] = stats.fetch("total_errors", 0) + arr.size
-      buckets = stats["error_buckets"] ||= { "auth" => 0, "api" => 0, "network" => 0, "other" => 0 }
+      Rails.logger.info("SimpleFin: recording #{arr.size} non-fatal provider error(s) with partial data present")
+      ActiveSupport::Notifications.instrument(
+        "simplefin.provider_errors",
+        item_id: simplefin_item.id,
+        count: arr.size
+      )
 
       arr.each do |error|
         msg = if error.is_a?(String)
@@ -520,14 +531,8 @@ class SimplefinItem::Importer
         else
           "other"
         end
-        buckets[category] = buckets.fetch(category, 0) + 1
-        stats["errors"] << { message: msg, category: category }
+        register_error(message: msg, category: category)
       end
-
-      # Keep error sample small for UI
-      stats["errors"] = stats["errors"].last(5)
-
-      persist_stats!
     end
 
     def handle_errors(errors)
@@ -544,17 +549,31 @@ class SimplefinItem::Importer
       end
 
       if needs_update
+        Rails.logger.warn("SimpleFin: marking item ##{simplefin_item.id} requires_update due to fatal auth error(s): #{error_messages}")
         simplefin_item.update!(status: :requires_update)
       end
 
+      down = error_messages.downcase
       # Detect and surface rate-limit specifically with a friendlier exception
-      if error_messages.downcase.include?("make fewer requests") ||
-         error_messages.downcase.include?("only refreshed once every 24 hours") ||
-         error_messages.downcase.include?("rate limit")
+      if down.include?("make fewer requests") ||
+         down.include?("only refreshed once every 24 hours") ||
+         down.include?("rate limit")
+        Rails.logger.info("SimpleFin: raising RateLimitedError for item ##{simplefin_item.id}: #{error_messages}")
+        ActiveSupport::Notifications.instrument(
+          "simplefin.rate_limited",
+          item_id: simplefin_item.id,
+          message: error_messages
+        )
         raise RateLimitedError, "SimpleFin rate limit: data refreshes at most once every 24 hours. Try again later."
       end
 
       # Fall back to generic SimpleFin error classified as :api_error
+      Rails.logger.error("SimpleFin fatal API error for item ##{simplefin_item.id}: #{error_messages}")
+      ActiveSupport::Notifications.instrument(
+        "simplefin.fatal_error",
+        item_id: simplefin_item.id,
+        message: error_messages
+      )
       raise Provider::Simplefin::SimplefinError.new(
         "SimpleFin API errors: #{error_messages}",
         :api_error

app/views/simplefin_items/_simplefin_item.html.erb

@@ -57,7 +57,6 @@
                   <% if tooltip_text.present? %>
                     <%= render DS::Tooltip.new(text: tooltip_text, icon: "alert-octagon", size: "sm", color: "warning") %>
                   <% end %>
-                  <%= render DS::Link.new(text: "View errors", icon: "alert-octagon", variant: "secondary", href: errors_simplefin_item_path(simplefin_item), frame: "modal") %>
                 <% end %>
 
                 <% if stats["total_accounts"].to_i > 0 %>

app/views/simplefin_items/errors.html.erb

@@ -1,29 +0,0 @@
-<%# Modal: Show SimpleFIN sync errors for a connection %>
-<%= turbo_frame_tag "modal" do %>
-  <%= render DS::Dialog.new do |dialog| %>
-    <% dialog.with_header(title: "SimpleFIN sync errors") %>
-
-    <% dialog.with_body do %>
-      <% if @errors.present? %>
-        <div class="p-2 text-sm text-secondary space-y-2">
-          <p>We found the following errors in the latest sync:</p>
-          <ul class="list-disc list-inside space-y-1">
-            <% @errors.each do |msg| %>
-              <li class="text-primary"><%= msg %></li>
-            <% end %>
-          </ul>
-        </div>
-      <% else %>
-        <div class="p-4 text-sm text-secondary">
-          <p>No errors were recorded for the latest sync.</p>
-        </div>
-      <% end %>
-    <% end %>
-
-    <% dialog.with_footer do %>
-      <div class="flex items-center justify-end gap-2">
-        <%= render DS::Link.new(text: "Close", variant: :secondary, href: accounts_path, data: { turbo_frame: "_top" }) %>
-      </div>
-    <% end %>
-  <% end %>
-<% end %>

config/routes.rb

@@ -303,7 +303,6 @@ Rails.application.routes.draw do
     member do
       post :sync
       post :balances
-      get :errors
       get :setup_accounts
       post :complete_account_setup
     end