Simplefin sync improvements (we-promise#240)

* Fix syncing issues with new connections and accounts..

- Keep SimpleFin institution metadata strictly per account (`simplefin_accounts.org_data`).
- Relax `simplefin_items` institution constraints to allow creating items before org data exists.
- Remove code that copied the first account’s `org` onto `simplefin_items`.

* Improve Simplefin Sync
•
SimpleFin: family “Sync” includes SimpleFin items; importer does unbounded discovery (with pending=1 fallback) before windowed fetch, for both regular and first syncs.
•
Stop populating item‑level institution fields; keep institution metadata per account.
•
Relax NOT NULL on item institution fields.
•
Post‑sync dashboard broadcasts are now guarded (UI cannot fail the job).
•
Show a friendly “daily refresh limit” banner on the SimpleFin card when the latest sync is rate‑limited.
•
Add bin/rails sure:simplefin:debug[ITEM_ID] to print latest sync, snapshot account count, simplefin_accounts count, and unlinked list.

* Fixed double‑quoted strings, spacing around array brackets and commas

* chore: ignore local .junie files

* - Broadcast error logs now include full backtraces
- SimpleFin discovery logic deduplicated fixed
- app/models/simplefin_item/importer.rb
--Added a concise docstring for perform_account_discovery describing purpose, steps, and side‑effects.
--Added a docstring for fetch_accounts_data describing params and return value.

Author: luckyPipewrench

.gitignore

@@ -106,3 +106,6 @@ scripts/
 .windsurfrules
 .cursor/rules/dev_workflow.mdc
 .cursor/rules/taskmaster.mdc
+
+# Local Junie helpers
+.junie/

app/models/family/sync_complete_event.rb

@@ -6,16 +6,27 @@ def initialize(family)
   end
 
   def broadcast
-    family.broadcast_replace(
-      target: "balance-sheet",
-      partial: "pages/dashboard/balance_sheet",
-      locals: { balance_sheet: family.balance_sheet }
-    )
+    # Dashboard partials can occasionally raise when rendered from background jobs
+    # (e.g., if intermediate series values are nil during a sync). Make broadcasts
+    # resilient so a post-sync UI refresh never causes the overall sync to report an error.
+    begin
+      family.broadcast_replace(
+        target: "balance-sheet",
+        partial: "pages/dashboard/balance_sheet",
+        locals: { balance_sheet: family.balance_sheet }
+      )
+    rescue => e
+      Rails.logger.error("Family::SyncCompleteEvent balance_sheet broadcast failed: #{e.message}\n#{e.backtrace&.join("\n")}")
+    end
 
-    family.broadcast_replace(
-      target: "net-worth-chart",
-      partial: "pages/dashboard/net_worth_chart",
-      locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days }
-    )
+    begin
+      family.broadcast_replace(
+        target: "net-worth-chart",
+        partial: "pages/dashboard/net_worth_chart",
+        locals: { balance_sheet: family.balance_sheet, period: Period.last_30_days }
+      )
+    rescue => e
+      Rails.logger.error("Family::SyncCompleteEvent net_worth_chart broadcast failed: #{e.message}\n#{e.backtrace&.join("\n")}")
+    end
   end
 end

app/models/family/syncer.rb

@@ -26,6 +26,6 @@ def perform_post_sync
 
   private
     def child_syncables
-      family.plaid_items + family.enable_banking_items + family.accounts.manual
+      family.plaid_items + family.simplefin_items.active + family.enable_banking_items + family.accounts.manual
     end
 end

app/models/simplefin_item.rb

@@ -54,13 +54,8 @@ def upsert_simplefin_snapshot!(accounts_snapshot)
       raw_payload: accounts_snapshot,
     )
 
-    # Extract institution data from the first account if available
-    snapshot = accounts_snapshot.to_h.with_indifferent_access
-    if snapshot[:accounts].present?
-      first_account = snapshot[:accounts].first
-      org = first_account[:org]
-      upsert_institution_data!(org) if org.present?
-    end
+    # Do not populate item-level institution fields from account data.
+    # Institution metadata belongs to each simplefin_account (in org_data).
 
     save!
   end
@@ -153,6 +148,26 @@ def institution_summary
     end
   end
 
+  # Detect a recent rate-limited sync and return a friendly message, else nil
+  def rate_limited_message
+    latest = latest_sync
+    return nil unless latest
+
+    # Some Sync records may not have a status_text column; guard with respond_to?
+    parts = []
+    parts << latest.error if latest.respond_to?(:error)
+    parts << latest.status_text if latest.respond_to?(:status_text)
+    msg = parts.compact.join(" — ")
+    return nil if msg.blank?
+
+    down = msg.downcase
+    if down.include?("make fewer requests") || down.include?("only refreshed once every 24 hours") || down.include?("rate limit")
+      "You've hit SimpleFin's daily refresh limit. Please try again after the bridge refreshes (up to 24 hours)."
+    else
+      nil
+    end
+  end
+
   private
     def remove_simplefin_item
       # SimpleFin doesn't require server-side cleanup like Plaid

app/models/simplefin_item/importer.rb

@@ -1,4 +1,5 @@
 class SimplefinItem::Importer
+  class RateLimitedError < StandardError; end
   attr_reader :simplefin_item, :simplefin_provider
 
   def initialize(simplefin_item, simplefin_provider:)
@@ -44,6 +45,10 @@ def import_with_chunked_history
         target_start_date = max_lookback_date
       end
 
+      # Pre-step: Unbounded discovery to ensure we see all accounts even if the
+      # chunked window would otherwise filter out newly added, inactive accounts.
+      perform_account_discovery
+
       total_accounts_imported = 0
       chunk_count = 0
 
@@ -100,30 +105,75 @@ def import_with_chunked_history
     end
 
     def import_regular_sync
-      start_date = determine_sync_start_date
+      perform_account_discovery
 
+      # Step 2: Fetch transactions/holdings using the regular window.
+      start_date = determine_sync_start_date
       accounts_data = fetch_accounts_data(start_date: start_date)
       return if accounts_data.nil? # Error already handled
 
       # Store raw payload
       simplefin_item.upsert_simplefin_snapshot!(accounts_data)
 
-      # Import accounts
+      # Import accounts (merges transactions/holdings into existing rows)
       accounts_data[:accounts]&.each do |account_data|
         import_account(account_data)
       end
     end
 
-    def fetch_accounts_data(start_date:, end_date: nil)
+    #
+    # Performs discovery of accounts in an unbounded way so providers that
+    # filter by date windows cannot hide newly created upstream accounts.
+    #
+    # Steps:
+    # - Request `/accounts` without dates; count results
+    # - If zero, retry with `pending: true` (some bridges only reveal new/pending)
+    # - If any accounts are returned, upsert a snapshot and import each account
+    #
+    # Returns nothing; side-effects are snapshot + account upserts.
+    def perform_account_discovery
+      discovery_data = fetch_accounts_data(start_date: nil)
+      discovered_count = discovery_data&.dig(:accounts)&.size.to_i
+      Rails.logger.info "SimpleFin discovery (no params) returned #{discovered_count} accounts"
+
+      if discovered_count.zero?
+        discovery_data = fetch_accounts_data(start_date: nil, pending: true)
+        discovered_count = discovery_data&.dig(:accounts)&.size.to_i
+        Rails.logger.info "SimpleFin discovery (pending=1) returned #{discovered_count} accounts"
+      end
+
+      if discovery_data && discovered_count > 0
+        simplefin_item.upsert_simplefin_snapshot!(discovery_data)
+        discovery_data[:accounts]&.each { |account_data| import_account(account_data) }
+      end
+    end
+
+    # Fetches accounts (and optionally transactions/holdings) from SimpleFin.
+    #
+    # Params:
+    # - start_date: Date or nil — when provided, provider may filter by date window
+    # - end_date:   Date or nil — optional end of window
+    # - pending:    Boolean or nil — when true, ask provider to include pending/new
+    #
+    # Returns a Hash payload with keys like :accounts, or nil when an error is
+    # handled internally via `handle_errors`.
+    def fetch_accounts_data(start_date:, end_date: nil, pending: nil)
       # Debug logging to track exactly what's being sent to SimpleFin API
-      days_requested = end_date ? (end_date.to_date - start_date.to_date).to_i : "unknown"
-      Rails.logger.info "SimplefinItem::Importer - API Request: #{start_date.strftime('%Y-%m-%d')} to #{end_date&.strftime('%Y-%m-%d') || 'current'} (#{days_requested} days)"
+      start_str = start_date.respond_to?(:strftime) ? start_date.strftime("%Y-%m-%d") : "none"
+      end_str = end_date.respond_to?(:strftime) ? end_date.strftime("%Y-%m-%d") : "current"
+      days_requested = if start_date && end_date
+        (end_date.to_date - start_date.to_date).to_i
+      else
+        "unknown"
+      end
+      Rails.logger.info "SimplefinItem::Importer - API Request: #{start_str} to #{end_str} (#{days_requested} days)"
 
       begin
         accounts_data = simplefin_provider.get_accounts(
           simplefin_item.access_url,
           start_date: start_date,
-          end_date: end_date
+          end_date: end_date,
+          pending: pending
         )
       rescue Provider::Simplefin::SimplefinError => e
         # Handle authentication errors by marking item as requiring update
@@ -141,6 +191,12 @@ def fetch_accounts_data(start_date:, end_date: nil)
         return nil
       end
 
+      # Some servers return a top-level message/string rather than an errors array
+      if accounts_data[:error].present?
+        handle_errors([ accounts_data[:error] ])
+        return nil
+      end
+
       accounts_data
     end
 
@@ -224,6 +280,13 @@ def handle_errors(errors)
         simplefin_item.update!(status: :requires_update)
       end
 
+      # 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")
+        raise RateLimitedError, "SimpleFin rate limit: data refreshes at most once every 24 hours. Try again later."
+      end
+
       raise Provider::Simplefin::SimplefinError.new(
         "SimpleFin API errors: #{error_messages}",
         :api_error

app/views/simplefin_items/_simplefin_item.html.erb

@@ -38,6 +38,11 @@
               <%= icon "alert-triangle", size: "sm", color: "warning" %>
               <%= tag.span t(".requires_update") %>
             </div>
+          <% elsif simplefin_item.rate_limited_message.present? %>
+            <div class="text-warning flex items-center gap-1">
+              <%= icon "clock", size: "sm", color: "warning" %>
+              <%= tag.span simplefin_item.rate_limited_message %>
+            </div>
           <% elsif simplefin_item.sync_error.present? %>
             <div class="text-secondary flex items-center gap-1">
               <%= render DS::Tooltip.new(text: simplefin_item.sync_error, icon: "alert-circle", size: "sm", color: "destructive") %>

db/migrate/20251025095800_relax_simplefin_item_institution_constraints.rb

@@ -0,0 +1,20 @@
+class RelaxSimplefinItemInstitutionConstraints < ActiveRecord::Migration[7.2]
+  def up
+    # SimpleFin doesn't guarantee institution metadata on first fetch,
+    # so these fields must be optional.
+    change_column_null :simplefin_items, :institution_id, true
+    change_column_null :simplefin_items, :institution_name, true
+  end
+
+  def down
+    # Restoring NOT NULL could break existing rows that legitimately have no institution metadata.
+    # We keep this reversible but conservative: only set NOT NULL if no NULLs exist.
+    if execute("SELECT COUNT(*) FROM simplefin_items WHERE institution_id IS NULL").first["count"].to_i == 0
+      change_column_null :simplefin_items, :institution_id, false
+    end
+
+    if execute("SELECT COUNT(*) FROM simplefin_items WHERE institution_name IS NULL").first["count"].to_i == 0
+      change_column_null :simplefin_items, :institution_name, false
+    end
+  end
+end

db/schema.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+namespace :sure do
+  namespace :simplefin do
+    desc "Print debug info for a SimpleFin item: latest sync, snapshot accounts, simplefin_accounts, and unlinked list"
+    task :debug, [ :item_id ] => :environment do |_, args|
+      unless args[:item_id].present?
+        puts({ error: "usage", example: "bin/rails sure:simplefin:debug[ITEM_ID]" }.to_json)
+        exit 1
+      end
+
+      item = SimplefinItem.find(args[:item_id])
+      latest_sync = item.syncs.order(created_at: :desc).first
+      # Our model stores the latest snapshot directly on the item (`raw_payload`).
+      snapshot_accounts = item.raw_payload&.dig(:accounts)&.size
+      unlinked = item.simplefin_accounts.left_joins(:account).where(accounts: { id: nil })
+
+      out = {
+        item_id: item.id,
+        name: item.name,
+        last_synced_at: item.last_synced_at,
+        latest_sync: latest_sync&.attributes&.slice("id", "status", "error", "status_text", "created_at", "completed_at"),
+        snapshot_accounts: snapshot_accounts,
+        simplefin_accounts_count: item.simplefin_accounts.count,
+        unlinked_count: unlinked.count,
+        unlinked: unlinked.limit(20).map { |sfa| { id: sfa.id, upstream_id: sfa.account_id, name: sfa.name } }
+      }
+
+      puts out.to_json
+    rescue => e
+      puts({ error: e.class.name, message: e.message, backtrace: e.backtrace&.take(3) }.to_json)
+      exit 1
+    end
+  end
+end