Improvements (#379)

* Improvements

- Fix button visibility in reports on light theme
- Unify logic for provider syncs
- Add default option is to skip accounts linking ( no op default )

* Stability fixes and UX improvements

* FIX add unlinking when deleting lunch flow connection as well

* Wrap updates in transaction

* Some more improvements

* FIX proper provider setup check

* Make provider section collapsible

* Fix balance calculation

* Restore focus ring

* Use browser default focus

* Fix lunch flow balance for credit cards

app/models/lunchflow_account/processor.rb

@@ -39,10 +39,15 @@ class LunchflowAccount::Processor
       account = lunchflow_account.current_account
       balance = lunchflow_account.current_balance || 0
 
-      # For liability accounts (credit cards and loans), ensure positive balances
-      # LunchFlow may return negative values for liabilities, but Sure expects positive
+      # LunchFlow balance convention matches our app convention:
+      # - Positive balance = debt (you owe money)
+      # - Negative balance = credit balance (bank owes you, e.g., overpayment)
+      # No sign conversion needed - pass through as-is (same as Plaid)
+      #
+      # Exception: CreditCard and Loan accounts return inverted signs
+      # Provider returns negative for positive balance, so we negate it
       if account.accountable_type == "CreditCard" || account.accountable_type == "Loan"
-        balance = balance.abs
+        balance = -balance
       end
 
       # Normalize currency with fallback chain: parsed lunchflow currency -> existing account currency -> USD

app/models/lunchflow_item.rb

@@ -1,5 +1,5 @@
 class LunchflowItem < ApplicationRecord
-  include Syncable, Provided
+  include Syncable, Provided, Unlinking
 
   enum :status, { good: "good", requires_update: "requires_update" }, default: :good
 
@@ -104,39 +104,32 @@ class LunchflowItem < ApplicationRecord
   end
 
   def sync_status_summary
-    latest = latest_sync
-    return nil unless latest
+    # Use centralized count helper methods for consistency
+    total_accounts = total_accounts_count
+    linked_count = linked_accounts_count
+    unlinked_count = unlinked_accounts_count
 
-    # If sync has statistics, use them
-    if latest.sync_stats.present?
-      stats = latest.sync_stats
-      total = stats["total_accounts"] || 0
-      linked = stats["linked_accounts"] || 0
-      unlinked = stats["unlinked_accounts"] || 0
-
-      if total == 0
-        "No accounts found"
-      elsif unlinked == 0
-        "#{linked} #{'account'.pluralize(linked)} synced"
-      else
-        "#{linked} synced, #{unlinked} need setup"
-      end
+    if total_accounts == 0
+      "No accounts found"
+    elsif unlinked_count == 0
+      "#{linked_count} #{'account'.pluralize(linked_count)} synced"
     else
-      # Fallback to current account counts
-      total_accounts = lunchflow_accounts.count
-      linked_count = accounts.count
-      unlinked_count = total_accounts - linked_count
-
-      if total_accounts == 0
-        "No accounts found"
-      elsif unlinked_count == 0
-        "#{linked_count} #{'account'.pluralize(linked_count)} synced"
-      else
-        "#{linked_count} synced, #{unlinked_count} need setup"
-      end
+      "#{linked_count} synced, #{unlinked_count} need setup"
     end
   end
 
+  def linked_accounts_count
+    lunchflow_accounts.joins(:account_provider).count
+  end
+
+  def unlinked_accounts_count
+    lunchflow_accounts.left_joins(:account_provider).where(account_providers: { id: nil }).count
+  end
+
+  def total_accounts_count
+    lunchflow_accounts.count
+  end
+
   def institution_display_name
     # Try to get institution name from stored metadata
     institution_name.presence || institution_domain.presence || name

app/models/lunchflow_item/unlinking.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module LunchflowItem::Unlinking
+  # Concern that encapsulates unlinking logic for a Lunchflow item.
+  # Mirrors the SimplefinItem::Unlinking behavior.
+  extend ActiveSupport::Concern
+
+  # Idempotently remove all connections between this Lunchflow item and local accounts.
+  # - Detaches any AccountProvider links for each LunchflowAccount
+  # - Detaches Holdings that point at the AccountProvider links
+  # Returns a per-account result payload for observability
+  def unlink_all!(dry_run: false)
+    results = []
+
+    lunchflow_accounts.find_each do |lfa|
+      links = AccountProvider.where(provider_type: "LunchflowAccount", provider_id: lfa.id).to_a
+      link_ids = links.map(&:id)
+      result = {
+        lfa_id: lfa.id,
+        name: lfa.name,
+        provider_link_ids: link_ids
+      }
+      results << result
+
+      next if dry_run
+
+      begin
+        ActiveRecord::Base.transaction do
+          # Detach holdings for any provider links found
+          if link_ids.any?
+            Holding.where(account_provider_id: link_ids).update_all(account_provider_id: nil)
+          end
+
+          # Destroy all provider links
+          links.each do |ap|
+            ap.destroy!
+          end
+        end
+      rescue => e
+        Rails.logger.warn(
+          "LunchflowItem Unlinker: failed to fully unlink LFA ##{lfa.id} (links=#{link_ids.inspect}): #{e.class} - #{e.message}"
+        )
+        # Record error for observability; continue with other accounts
+        result[:error] = e.message
+      end
+    end
+
+    results
+  end
+end

app/models/provider/configurable.rb

@@ -113,6 +113,7 @@ module Provider::Configurable
       @provider_key = provider_key
       @fields = []
       @provider_description = nil
+      @configured_check = nil
     end
 
     # Set the provider-level description (markdown supported)
@@ -121,6 +122,14 @@ module Provider::Configurable
       @provider_description = text
     end
 
+    # Define a custom check for whether this provider is configured
+    # @param block [Proc] A block that returns true if the provider is configured
+    # Example:
+    #   configured_check { get_value(:client_id).present? && get_value(:secret).present? }
+    def configured_check(&block)
+      @configured_check = block
+    end
+
     # Define a configuration field
     # @param name [Symbol] The field name
     # @param label [String] Human-readable label
@@ -150,9 +159,21 @@ module Provider::Configurable
       field.value
     end
 
-    # Check if all required fields are present
+    # Check if provider is properly configured
+    # Uses custom configured_check if defined, otherwise checks required fields
     def configured?
-      fields.select(&:required).all? { |f| f.value.present? }
+      if @configured_check
+        instance_eval(&@configured_check)
+      else
+        required_fields = fields.select(&:required)
+        if required_fields.any?
+          required_fields.all? { |f| f.value.present? }
+        else
+          # If no required fields, provider is not considered configured
+          # unless it defines a custom configured_check
+          false
+        end
+      end
     end
 
     # Get all field values as a hash

app/models/provider/plaid_adapter.rb

@@ -106,6 +106,9 @@ class Provider::PlaidAdapter < Provider::Base
           env_key: "PLAID_ENV",
           default: "sandbox",
           description: "Plaid environment: sandbox, development, or production"
+
+    # Plaid requires both client_id and secret to be configured
+    configured_check { get_value(:client_id).present? && get_value(:secret).present? }
   end
 
   def provider_name

app/models/provider/plaid_eu_adapter.rb

@@ -45,6 +45,9 @@ class Provider::PlaidEuAdapter
           env_key: "PLAID_EU_ENV",
           default: "sandbox",
           description: "Plaid environment: sandbox, development, or production"
+
+    # Plaid EU requires both client_id and secret to be configured
+    configured_check { get_value(:client_id).present? && get_value(:secret).present? }
   end
 
   # Thread-safe lazy loading of Plaid EU configuration

app/models/simplefin_account/processor.rb

@@ -41,11 +41,10 @@ class SimplefinAccount::Processor
       account = simplefin_account.current_account
       balance = simplefin_account.current_balance || simplefin_account.available_balance || 0
 
-      # SimpleFin returns negative balances for credit cards (liabilities)
-      # But Maybe expects positive balances for liabilities
-      if account.accountable_type == "CreditCard" || account.accountable_type == "Loan"
-        balance = balance.abs
-      end
+      # SimpleFIN balance convention matches our app convention:
+      # - Positive balance = debt (you owe money)
+      # - Negative balance = credit balance (bank owes you, e.g., overpayment)
+      # No sign conversion needed - pass through as-is (same as Plaid)
 
       # Calculate cash balance correctly for investment accounts
       cash_balance = if account.accountable_type == "Investment"

app/models/sync.rb

@@ -67,6 +67,24 @@ class Sync < ApplicationRecord
         return
       end
 
+      # Guard: syncable may have been deleted while job was queued
+      unless syncable.present?
+        Rails.logger.warn("Sync #{id} - syncable #{syncable_type}##{syncable_id} no longer exists. Marking as failed.")
+        start! if may_start?
+        fail!
+        update(error: "Syncable record was deleted")
+        return
+      end
+
+      # Guard: syncable may be scheduled for deletion
+      if syncable.respond_to?(:scheduled_for_deletion?) && syncable.scheduled_for_deletion?
+        Rails.logger.warn("Sync #{id} - syncable #{syncable_type}##{syncable_id} is scheduled for deletion. Skipping sync.")
+        start! if may_start?
+        fail!
+        update(error: "Syncable record is scheduled for deletion")
+        return
+      end
+
       start!
 
       begin