feat: Enhance holding detail drawer with live price sync and enriched overview (#1086)

* Feat: Implement manual sync prices functionality and enhance holdings display

* Feat: Enhance sync prices functionality with error handling and update UI components

* Feat: Update sync prices error handling and enhance Spanish locale messages

* Fix: Address CodeRabbit review feedback

- Set fallback @provider_error when prices_updated == 0 so turbo stream
  never fails silently without a visible error message
- Move attr_reader :provider_error to class header in Price::Importer
  for conventional placement alongside other attribute declarations
- Precompute @last_price_updated in controller (show + sync_prices)
  instead of running a DB query directly inside ERB templates

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix: Replace bare rescue with explicit exception handling in turbo stream view

Bare `rescue` silently swallows all exceptions, making debugging impossible.
Match the pattern already used in show.html.erb: rescue ActiveRecord::RecordInvalid
explicitly, then catch StandardError with logging (message + backtrace) before
falling back to the unknown label.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix: Update test assertion to expect actual provider error message

The stub returns "Yahoo Finance rate limit exceeded" as the provider error.
After the @provider_error fallback fix, the controller now correctly surfaces
the real provider error when present (using .presence || fallback), so the
flash[:alert] is the actual error string, not the generic fallback.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* Fix: Assert scoped security_ids in sync_prices materializer test

Replace loose stub with constructor expectation to verify that
Balance::Materializer is instantiated with the single-security scope.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix: Assert holding remap in remap_security test

Add assertion that @holding.security_id is updated to the target
security after remap, covering the core command outcome.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Fix: CI test failure - Update disconnect external assistant test to use env overrides

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Gemfile.lock

@@ -441,7 +441,7 @@ GEM
     ostruct (0.6.2)
     pagy (9.3.5)
     parallel (1.27.0)
-    parser (3.3.8.0)
+    parser (3.3.10.2)
       ast (~> 2.4.1)
       racc
     pdf-reader (2.15.1)

app/controllers/holdings_controller.rb

@@ -1,11 +1,12 @@
 class HoldingsController < ApplicationController
-  before_action :set_holding, only: %i[show update destroy unlock_cost_basis remap_security reset_security]
+  before_action :set_holding, only: %i[show update destroy unlock_cost_basis remap_security reset_security sync_prices]
 
   def index
     @account = Current.family.accounts.find(params[:account_id])
   end
 
   def show
+    @last_price_updated = @holding.security.prices.maximum(:updated_at)
   end
 
   def update
@@ -70,6 +71,13 @@ class HoldingsController < ApplicationController
       return
     end
 
+    # The user explicitly selected this security from provider search results,
+    # so we know the provider can handle it. Bring it back online if it was
+    # previously marked offline (e.g. by a failed QIF import resolution).
+    if new_security.offline?
+      new_security.update!(offline: false, failed_fetch_count: 0, failed_fetch_at: nil)
+    end
+
     @holding.remap_security!(new_security)
     flash[:notice] = t(".success")
 
@@ -79,6 +87,44 @@ class HoldingsController < ApplicationController
     end
   end
 
+  def sync_prices
+    security = @holding.security
+
+    if security.offline?
+      redirect_to account_path(@holding.account, tab: "holdings"),
+                  alert: t("holdings.sync_prices.unavailable")
+      return
+    end
+
+    prices_updated, @provider_error = security.import_provider_prices(
+      start_date: 31.days.ago.to_date,
+      end_date: Date.current,
+      clear_cache: true
+    )
+    security.import_provider_details
+
+    @last_price_updated = @holding.security.prices.maximum(:updated_at)
+
+    if prices_updated == 0
+      @provider_error = @provider_error.presence || t("holdings.sync_prices.provider_error")
+      respond_to do |format|
+        format.html { redirect_to account_path(@holding.account, tab: "holdings"), alert: @provider_error }
+        format.turbo_stream
+      end
+      return
+    end
+
+    strategy = @holding.account.linked? ? :reverse : :forward
+    Balance::Materializer.new(@holding.account, strategy: strategy, security_ids: [ @holding.security_id ]).materialize_balances
+    @holding.reload
+    @last_price_updated = @holding.security.prices.maximum(:updated_at)
+
+    respond_to do |format|
+      format.html { redirect_to account_path(@holding.account, tab: "holdings"), notice: t("holdings.sync_prices.success") }
+      format.turbo_stream
+    end
+  end
+
   def reset_security
     @holding.reset_security_to_provider!
     flash[:notice] = t(".success")

app/models/balance/materializer.rb

@@ -1,9 +1,10 @@
 class Balance::Materializer
-  attr_reader :account, :strategy
+  attr_reader :account, :strategy, :security_ids
 
-  def initialize(account, strategy:)
+  def initialize(account, strategy:, security_ids: nil)
     @account = account
     @strategy = strategy
+    @security_ids = security_ids
   end
 
   def materialize_balances
@@ -24,7 +25,7 @@ class Balance::Materializer
 
   private
     def materialize_holdings
-      @holdings = Holding::Materializer.new(account, strategy: strategy).materialize_holdings
+      @holdings = Holding::Materializer.new(account, strategy: strategy, security_ids: security_ids).materialize_holdings
     end
 
     def update_account_info

app/models/holding/forward_calculator.rb

@@ -1,8 +1,9 @@
 class Holding::ForwardCalculator
   attr_reader :account
 
-  def initialize(account)
+  def initialize(account, security_ids: nil)
     @account = account
+    @security_ids = security_ids
     # Track cost basis per security: { security_id => { total_cost: BigDecimal, total_qty: BigDecimal } }
     @cost_basis_tracker = Hash.new { |h, k| h[k] = { total_cost: BigDecimal("0"), total_qty: BigDecimal("0") } }
   end
@@ -27,7 +28,7 @@ class Holding::ForwardCalculator
 
   private
     def portfolio_cache
-      @portfolio_cache ||= Holding::PortfolioCache.new(account)
+      @portfolio_cache ||= Holding::PortfolioCache.new(account, security_ids: @security_ids)
     end
 
     def empty_portfolio
@@ -55,6 +56,8 @@ class Holding::ForwardCalculator
 
     def build_holdings(portfolio, date, price_source: nil)
       portfolio.map do |security_id, qty|
+        next if @security_ids && !@security_ids.include?(security_id)
+
         price = portfolio_cache.get_price(security_id, date, source: price_source)
 
         if price.nil?

app/models/holding/materializer.rb

@@ -1,9 +1,10 @@
 # "Materializes" holdings (similar to a DB materialized view, but done at the app level)
 # into a series of records we can easily query and join with other data.
 class Holding::Materializer
-  def initialize(account, strategy:)
+  def initialize(account, strategy:, security_ids: nil)
     @account = account
     @strategy = strategy
+    @security_ids = security_ids
   end
 
   def materialize_holdings
@@ -12,7 +13,7 @@ class Holding::Materializer
     Rails.logger.info("Persisting #{@holdings.size} holdings")
     persist_holdings
 
-    if strategy == :forward
+    if strategy == :forward && security_ids.nil?
       purge_stale_holdings
     end
 
@@ -28,7 +29,7 @@ class Holding::Materializer
   end
 
   private
-    attr_reader :account, :strategy
+    attr_reader :account, :strategy, :security_ids
 
     def calculate_holdings
       @holdings = calculator.calculate
@@ -164,9 +165,9 @@ class Holding::Materializer
     def calculator
       if strategy == :reverse
         portfolio_snapshot = Holding::PortfolioSnapshot.new(account)
-        Holding::ReverseCalculator.new(account, portfolio_snapshot: portfolio_snapshot)
+        Holding::ReverseCalculator.new(account, portfolio_snapshot: portfolio_snapshot, security_ids: security_ids)
       else
-        Holding::ForwardCalculator.new(account)
+        Holding::ForwardCalculator.new(account, security_ids: security_ids)
       end
     end
 end

app/models/holding/portfolio_cache.rb

@@ -7,9 +7,10 @@ class Holding::PortfolioCache
     end
   end
 
-  def initialize(account, use_holdings: false)
+  def initialize(account, use_holdings: false, security_ids: nil)
     @account = account
     @use_holdings = use_holdings
+    @security_ids = security_ids
     load_prices
   end
 
@@ -62,10 +63,12 @@ class Holding::PortfolioCache
 
     def collect_unique_securities
       unique_securities_from_trades = trades.map(&:entryable).map(&:security).uniq
+      unique_securities_from_trades = unique_securities_from_trades.select { |s| @security_ids.include?(s.id) } if @security_ids
 
       return unique_securities_from_trades unless use_holdings
 
       unique_securities_from_holdings = holdings.map(&:security).uniq
+      unique_securities_from_holdings = unique_securities_from_holdings.select { |s| @security_ids.include?(s.id) } if @security_ids
 
       (unique_securities_from_trades + unique_securities_from_holdings).uniq
     end

app/models/holding/reverse_calculator.rb

@@ -1,9 +1,10 @@
 class Holding::ReverseCalculator
   attr_reader :account, :portfolio_snapshot
 
-  def initialize(account, portfolio_snapshot:)
+  def initialize(account, portfolio_snapshot:, security_ids: nil)
     @account = account
     @portfolio_snapshot = portfolio_snapshot
+    @security_ids = security_ids
   end
 
   def calculate
@@ -19,7 +20,7 @@ class Holding::ReverseCalculator
     # since it is common for a provider to supply "current day" holdings but not all the historical
     # trades that make up those holdings.
     def portfolio_cache
-      @portfolio_cache ||= Holding::PortfolioCache.new(account, use_holdings: true)
+      @portfolio_cache ||= Holding::PortfolioCache.new(account, use_holdings: true, security_ids: @security_ids)
     end
 
     def calculate_holdings
@@ -57,6 +58,8 @@ class Holding::ReverseCalculator
 
     def build_holdings(portfolio, date, price_source: nil)
       portfolio.map do |security_id, qty|
+        next if @security_ids && !@security_ids.include?(security_id)
+
         price = portfolio_cache.get_price(security_id, date, source: price_source)
 
         if price.nil?

app/models/security/price/importer.rb

@@ -4,6 +4,8 @@ class Security::Price::Importer
 
   PROVISIONAL_LOOKBACK_DAYS = 7
 
+  attr_reader :provider_error
+
   def initialize(security:, security_provider:, start_date:, end_date:, clear_cache: false)
     @security          = security
     @security_provider = security_provider
@@ -130,6 +132,7 @@ class Security::Price::Importer
             scope.set_context("security", { id: security.id, start_date: start_date, end_date: end_date })
           end
 
+          @provider_error = error_message
           {}
         end
       end

app/models/security/provided.rb

@@ -114,13 +114,14 @@ module Security::Provided
       return 0
     end
 
-    Security::Price::Importer.new(
+    importer = Security::Price::Importer.new(
       security: self,
       security_provider: provider,
       start_date: start_date,
       end_date: end_date,
       clear_cache: clear_cache
-    ).import_provider_prices
+    )
+    [ importer.import_provider_prices, importer.provider_error ]
   end
 
   private

app/views/holdings/show.html.erb

@@ -35,7 +35,7 @@
                 <button type="button"
                         data-action="click->holding-security-remap#toggle"
                   class="text-secondary hover:text-primary"
-                  aria-label="<%= t('.edit_security') %>">
+                  aria-label="<%= t(".edit_security") %>">
                   <%= icon "pencil", size: "xs" %>
                 </button>
               </dd>
@@ -66,7 +66,7 @@
           </div>
           <div class="flex items-center justify-between text-sm">
             <dt class="text-secondary"><%= t(".current_market_price_label") %></dt>
-            <dd class="text-primary">
+            <dd id="<%= dom_id(@holding, :current_market_price) %>" class="text-primary">
               <% begin %>
                 <%= @holding.security.current_price ? format_money(@holding.security.current_price) : t(".unknown") %>
                 <% rescue ActiveRecord::RecordInvalid %>
@@ -78,6 +78,10 @@
               <% end %>
             </dd>
           </div>
+          <div class="flex items-center justify-between text-sm">
+            <dt class="text-secondary"><%= t(".shares_label") %></dt>
+            <dd class="text-primary"><%= format_quantity(@holding.qty) %></dd>
+          </div>
           <div class="flex items-center justify-between text-sm">
             <dt class="text-secondary"><%= t(".portfolio_weight_label") %></dt>
             <dd class="text-primary"><%= @holding.weight ? number_to_percentage(@holding.weight, precision: 2) : t(".unknown") %></dd>
@@ -171,6 +175,17 @@
             </div>
           </div>
           <div class="flex items-center justify-between text-sm">
+            <dt class="text-secondary"><%= t(".book_value_label") %></dt>
+            <dd class="text-primary">
+              <% book_value = @holding.avg_cost ? @holding.avg_cost * @holding.qty : nil %>
+              <%= book_value ? format_money(book_value) : t(".unknown") %>
+            </dd>
+          </div>
+          <div id="<%= dom_id(@holding, :market_value) %>" class="flex items-center justify-between text-sm">
+            <dt class="text-secondary"><%= t(".market_value_label") %></dt>
+            <dd class="text-primary"><%= format_money(@holding.amount_money) %></dd>
+          </div>
+          <div id="<%= dom_id(@holding, :total_return) %>" class="flex items-center justify-between text-sm">
             <dt class="text-secondary"><%= t(".total_return_label") %></dt>
             <% if @holding.trend %>
               <dd style="color: <%= @holding.trend.color %>;">
@@ -214,7 +229,7 @@
         </div>
       </div>
     <% end %>
-    <% if @holding.cost_basis_locked? || @holding.security_remapped? || @holding.account.can_delete_holdings? %>
+    <% if @holding.cost_basis_locked? || @holding.security_remapped? || @holding.account.can_delete_holdings? || !@holding.security.offline? %>
       <% dialog.with_section(title: t(".settings"), open: true) do %>
         <div class="pb-4">
           <% if @holding.security_remapped? %>
@@ -234,6 +249,26 @@
                   } } %>
             </div>
           <% end %>
+          <% unless @holding.security.offline? %>
+            <div id="<%= dom_id(@holding, :market_data_section) %>" class="flex items-center justify-between gap-2 p-3 border-b border-tertiary">
+              <div class="text-sm space-y-1">
+                <h4 class="text-primary"><%= t(".market_data_label") %></h4>
+                <p class="text-secondary">
+                  <%= t(".last_price_update") %>: <%= @last_price_updated ? l(@last_price_updated, format: :long) : t(".never") %>
+                </p>
+              </div>
+              <%= button_to t(".market_data_sync_button"),
+                  sync_prices_holding_path(@holding),
+                  method: :post,
+                  class: "inline-flex items-center gap-1 px-3 py-2 rounded-lg text-sm font-medium text-primary bg-gray-200 hover:bg-gray-300 theme-dark:bg-gray-700 theme-dark:hover:bg-gray-600",
+                  data: { loading_button_target: "button" },
+                  form: { data: {
+                    controller: "loading-button",
+                    action: "submit->loading-button#showLoading",
+                    loading_button_loading_text_value: t(".syncing")
+                  } } %>
+            </div>
+          <% end %>
           <% if @holding.cost_basis_locked? %>
             <div class="flex items-center justify-between gap-2 p-3 border-b border-tertiary">
               <div class="text-sm space-y-1">

app/views/holdings/sync_prices.turbo_stream.erb

@@ -0,0 +1,56 @@
+<% unless @provider_error %>
+  <%= turbo_stream.replace dom_id(@holding, :current_market_price) do %>
+    <dd id="<%= dom_id(@holding, :current_market_price) %>" class="text-primary">
+      <% begin %>
+        <%= @holding.security.current_price ? format_money(@holding.security.current_price) : t("holdings.show.unknown") %>
+        <% rescue ActiveRecord::RecordInvalid %>
+        <%= t("holdings.show.unknown") %>
+        <% rescue StandardError => e %>
+        <% logger.error "Error fetching current price for security #{@holding.security.id}: #{e.message}" %>
+        <% logger.error e.backtrace.first(5).join("\n") %>
+        <%= t("holdings.show.unknown") %>
+      <% end %>
+    </dd>
+  <% end %>
+  <%= turbo_stream.replace dom_id(@holding, :market_value) do %>
+    <div id="<%= dom_id(@holding, :market_value) %>" class="flex items-center justify-between text-sm">
+      <dt class="text-secondary"><%= t("holdings.show.market_value_label") %></dt>
+      <dd class="text-primary"><%= format_money(@holding.amount_money) %></dd>
+    </div>
+  <% end %>
+  <%= turbo_stream.replace dom_id(@holding, :total_return) do %>
+    <div id="<%= dom_id(@holding, :total_return) %>" class="flex items-center justify-between text-sm">
+      <dt class="text-secondary"><%= t("holdings.show.total_return_label") %></dt>
+      <% if @holding.trend %>
+        <dd style="color: <%= @holding.trend.color %>;">
+          <%= render("shared/trend_change", trend: @holding.trend) %>
+        </dd>
+      <% else %>
+        <dd class="text-secondary"><%= t("holdings.show.unknown") %></dd>
+      <% end %>
+    </div>
+  <% end %>
+<% end %>
+<%= turbo_stream.replace dom_id(@holding, :market_data_section) do %>
+  <div id="<%= dom_id(@holding, :market_data_section) %>" class="flex items-center justify-between gap-2 p-3 border-b border-tertiary">
+    <div class="text-sm space-y-1">
+      <h4 class="text-primary"><%= t("holdings.show.market_data_label") %></h4>
+      <p class="text-secondary">
+        <%= t("holdings.show.last_price_update") %>: <%= @last_price_updated ? l(@last_price_updated, format: :long) : t("holdings.show.never") %>
+      </p>
+      <% if @provider_error %>
+        <p class="text-xs text-red-500"><%= @provider_error %></p>
+      <% end %>
+    </div>
+    <%= button_to t("holdings.show.market_data_sync_button"),
+        sync_prices_holding_path(@holding),
+        method: :post,
+        class: "inline-flex items-center gap-1 px-3 py-2 rounded-lg text-sm font-medium text-primary bg-gray-200 hover:bg-gray-300 theme-dark:bg-gray-700 theme-dark:hover:bg-gray-600",
+        data: { loading_button_target: "button" },
+        form: { data: {
+          controller: "loading-button",
+          action: "submit->loading-button#showLoading",
+          loading_button_loading_text_value: t("holdings.show.syncing")
+        } } %>
+  </div>
+<% end %>

config/locales/views/holdings/de.yml

@@ -32,4 +32,7 @@ de:
       ticker_label: Ticker
       trade_history_entry: "%{qty} Anteile von %{security} zu %{price}"
       total_return_label: Gesamtrendite
+      shares_label: Anteile
+      book_value_label: Buchwert
+      market_value_label: Marktwert
       unknown: Unbekannt

config/locales/views/holdings/en.yml

@@ -15,6 +15,10 @@ en:
       security_not_found: Could not find the selected security.
     reset_security:
       success: Security reset to provider value.
+    sync_prices:
+      success: Market data synced successfully.
+      unavailable: Market data sync is not available for offline securities.
+      provider_error: Could not fetch latest prices. Please try again in a few minutes.
     errors:
       security_collision: "Cannot remap: you already have a holding for %{ticker} on %{date}."
     cost_basis_sources:
@@ -82,3 +86,11 @@ en:
       unlock_cost_basis: Unlock
       unlock_confirm_title: Unlock cost basis?
       unlock_confirm_body: This will allow the cost basis to be updated by provider syncs or trade calculations.
+      shares_label: Shares
+      book_value_label: Book Value
+      market_value_label: Market Value
+      market_data_label: Market data
+      market_data_sync_button: Refresh
+      last_price_update: Last price update
+      syncing: Syncing...
+      never: Never

config/locales/views/holdings/es.yml

@@ -47,6 +47,10 @@ es:
     missing_price_tooltip:
       description: Esta inversión tiene valores faltantes y no pudimos calcular su rendimiento o valor.
       missing_data: Datos faltantes
+    sync_prices:
+      success: Datos de mercado sincronizados correctamente.
+      unavailable: La sincronización de datos de mercado no está disponible para valores fuera de línea.
+      provider_error: No se pudieron obtener los precios más recientes. Inténtalo de nuevo en unos minutos.
     show:
       avg_cost_label: Costo promedio
       current_market_price_label: Precio de mercado actual
@@ -74,6 +78,14 @@ es:
       ticker_label: Ticker
       trade_history_entry: "%{qty} acciones de %{security} a %{price}"
       total_return_label: Rendimiento total
+      shares_label: Acciones
+      book_value_label: Valor en libros
+      market_value_label: Valor de mercado
+      market_data_label: Datos de mercado
+      market_data_sync_button: Actualizar
+      last_price_update: Última actualización de precio
+      syncing: Sincronizando...
+      never: Nunca
       unknown: Desconocido
       cost_basis_locked_label: La base de costes está bloqueada
       cost_basis_locked_description: La base de costes establecida manualmente no cambiará con las sincronizaciones.

config/locales/views/holdings/fr.yml

@@ -33,4 +33,7 @@ fr:
       ticker_label: Ticker
       trade_history_entry: "%{qty} actions de %{security} à %{price}"
       total_return_label: Rendement total
+      shares_label: Actions
+      book_value_label: Valeur comptable
+      market_value_label: Valeur marchande
       unknown: Inconnu

config/routes.rb

@@ -247,6 +247,7 @@ Rails.application.routes.draw do
       post :unlock_cost_basis
       patch :remap_security
       post :reset_security
+      post :sync_prices
     end
   end
   resources :trades, only: %i[show new create update destroy] do

test/controllers/holdings_controller_test.rb

@@ -61,4 +61,61 @@ class HoldingsControllerTest < ActionDispatch::IntegrationTest
     assert_equal 50.0, @holding.cost_basis.to_f
     assert_equal "manual", @holding.cost_basis_source
   end
+
+  test "remap_security brings offline security back online" do
+    # Given: the target security is marked offline (e.g. created by a failed QIF import)
+    msft = securities(:msft)
+    msft.update!(offline: true, failed_fetch_count: 3)
+
+    # When: user explicitly selects it from the provider search and saves
+    patch remap_security_holding_path(@holding), params: { security_id: "MSFT|XNAS" }
+
+    # Then: the security is brought back online and the holding is remapped
+    assert_redirected_to account_path(@holding.account, tab: "holdings")
+    @holding.reload
+    msft.reload
+    assert_equal msft.id, @holding.security_id
+    assert_not msft.offline?
+    assert_equal 0, msft.failed_fetch_count
+  end
+
+  test "sync_prices redirects with alert for offline security" do
+    @holding.security.update!(offline: true)
+
+    post sync_prices_holding_path(@holding)
+
+    assert_redirected_to account_path(@holding.account, tab: "holdings")
+    assert_equal I18n.t("holdings.sync_prices.unavailable"), flash[:alert]
+  end
+
+  test "sync_prices syncs market data and redirects with notice" do
+    Security.any_instance.expects(:import_provider_prices).with(
+      start_date: 31.days.ago.to_date,
+      end_date: Date.current,
+      clear_cache: true
+    ).returns([ 31, nil ])
+    Security.any_instance.stubs(:import_provider_details)
+    materializer = mock("materializer")
+    materializer.expects(:materialize_balances).once
+    Balance::Materializer.expects(:new).with(
+      @holding.account,
+      strategy: :forward,
+      security_ids: [ @holding.security_id ]
+    ).returns(materializer)
+
+    post sync_prices_holding_path(@holding)
+
+    assert_redirected_to account_path(@holding.account, tab: "holdings")
+    assert_equal I18n.t("holdings.sync_prices.success"), flash[:notice]
+  end
+
+  test "sync_prices shows provider error inline when provider returns no prices" do
+    Security.any_instance.stubs(:import_provider_prices).returns([ 0, "Yahoo Finance rate limit exceeded" ])
+    Security.any_instance.stubs(:import_provider_details)
+
+    post sync_prices_holding_path(@holding)
+
+    assert_redirected_to account_path(@holding.account, tab: "holdings")
+    assert_equal "Yahoo Finance rate limit exceeded", flash[:alert]
+  end
 end

test/controllers/settings/hostings_controller_test.rb

@@ -201,16 +201,18 @@ class Settings::HostingsControllerTest < ActionDispatch::IntegrationTest
 
   test "disconnect external assistant clears settings and resets type" do
     with_self_hosting do
-      Setting.external_assistant_url = "https://agent.example.com/v1/chat"
-      Setting.external_assistant_token = "token"
-      Setting.external_assistant_agent_id = "finance-bot"
-      users(:family_admin).family.update!(assistant_type: "external")
+      with_env_overrides("EXTERNAL_ASSISTANT_URL" => nil, "EXTERNAL_ASSISTANT_TOKEN" => nil) do
+        Setting.external_assistant_url = "https://agent.example.com/v1/chat"
+        Setting.external_assistant_token = "token"
+        Setting.external_assistant_agent_id = "finance-bot"
+        users(:family_admin).family.update!(assistant_type: "external")
 
-      delete disconnect_external_assistant_settings_hosting_url
+        delete disconnect_external_assistant_settings_hosting_url
 
-      assert_redirected_to settings_hosting_url
-      assert_not Assistant::External.configured?
-      assert_equal "builtin", users(:family_admin).family.reload.assistant_type
+        assert_redirected_to settings_hosting_url
+        assert_not Assistant::External.configured?
+        assert_equal "builtin", users(:family_admin).family.reload.assistant_type
+      end
     end
   ensure
     Setting.external_assistant_url = nil