block · lily-de · Mar 12, 2025 · Mar 11, 2025 · Mar 11, 2025 · Mar 11, 2025
diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs
@@ -11,9 +11,9 @@ use http::{HeaderMap, StatusCode};
 use serde::{Deserialize, Serialize};
 use serde_json::Value;
 use std::collections::HashMap;
-use std::env;
 use utoipa::ToSchema;
 
+use crate::routes::utils::check_provider_configured;
 use crate::state::AppState;
 
 fn verify_secret_key(headers: &HeaderMap, state: &AppState) -> Result<StatusCode, StatusCode> {
@@ -123,7 +123,7 @@ pub async fn remove_config(
 }
 
 #[utoipa::path(
-    post, // Change from get to post
+    post,
     path = "/config/read",
     request_body = ConfigKeyQuery, // Switch back to request_body
     responses(
@@ -335,31 +335,6 @@ pub async fn providers(
     Ok(Json(providers_response))
 }
 
-fn check_provider_configured(metadata: &ProviderMetadata) -> bool {
-    let config = Config::global();
-
-    // Check all required keys for the provider
-    for key in &metadata.config_keys {
-        if key.required {
-            let key_name = &key.name;
-
-            // First, check if the key is set in the environment
-            let is_set_in_env = env::var(key_name).is_ok();
-
-            // If not set in environment, check the config file based on whether it's a secret or not
-            let is_set_in_config = config.get(key_name, key.secret).is_ok();
-
-            // If the key is neither in the environment nor in the config, the provider is not configured
-            if !is_set_in_env && !is_set_in_config {
-                return false;
-            }
-        }
-    }
-
-    // If all required keys are accounted for, the provider is considered configured
-    true
-}
-
 pub fn routes(state: AppState) -> Router {
     Router::new()
         .route("/config", get(read_all_config))

diff --git a/crates/goose-server/src/routes/mod.rs b/crates/goose-server/src/routes/mod.rs
@@ -6,7 +6,7 @@ pub mod extension;
 pub mod health;
 pub mod reply;
 pub mod session;
-
+pub mod utils;
 use axum::Router;
 
 // Function to configure all routes

diff --git a/crates/goose-server/src/routes/utils.rs b/crates/goose-server/src/routes/utils.rs
@@ -1,13 +1,15 @@
+use goose::config::Config;
+use goose::providers::base::{ConfigKey, ProviderMetadata};
 use serde::{Deserialize, Serialize};
+use std::env;
 use std::error::Error;
-use goose::config::Config;
 
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
 pub enum KeyLocation {
     Environment,
     ConfigFile,
     Keychain,
-    NotFound
+    NotFound,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ 
8000
-20,10 +22,8 @@ pub struct KeyInfo {
 }
 
 /// Inspects a configuration key to determine if it's set, its location, and value (for non-secret keys)
-pub fn inspect_key(
-    key_name: &str,
-    is_secret: bool,
-) -> Result<KeyInfo, Box<dyn Error>> {
+#[allow(dead_code)]
+pub fn inspect_key(key_name: &str, is_secret: bool) -> Result<KeyInfo, Box<dyn Error>> {
     let config = Config::global();
 
     // Check environment variable first
@@ -44,7 +44,7 @@ pub fn inspect_key(
     let config_result = if is_secret {
         config.get_secret(key_name).map(|v| (v, true))
     } else {
-        config.get(key_name).map(|v| (v, false))
+        config.get_param(key_name).map(|v| (v, false))
     };
 
     match config_result {
@@ -64,20 +64,19 @@ pub fn inspect_key(
                 // Only include value for non-secret keys
                 value: if !is_secret_actual { Some(value) } else { None },
             })
-        },
-        Err(_) => {
-            Ok(KeyInfo {
-                name: key_name.to_string(),
-                is_set: false,
-                location: KeyLocation::NotFound,
-                is_secret,
-                value: None,
-            })
         }
+        Err(_) => Ok(KeyInfo {
+            name: key_name.to_string(),
+            is_set: false,
+            location: KeyLocation::NotFound,
+            is_secret,
+            value: None,
+        }),
     }
 }
 
 /// Inspects multiple keys at once
+#[allow(dead_code)]
 pub fn inspect_keys(
     keys: &[(String, bool)], // (name, is_secret) pairs
 ) -> Result<Vec<KeyInfo>, Box<dyn Error>> {
@@ -89,4 +88,53 @@ pub fn inspect_keys(
     }
 
     Ok(results)
-}
+}
+
+pub fn check_provider_configured(metadata: &ProviderMetadata) -> bool {
+    let config = Config::global();
+
+    // Get all required keys
+    let required_keys: Vec<&ConfigKey> = metadata
+        .config_keys
+        .iter()
+        .filter(|key| key.required)
+        .collect();
+
+    // Special case: If a provider has exactly one required key and that key
+    // has a default value, check if it's explicitly set
+    if required_keys.len() == 1 && required_keys[0].default.is_some() {
+        let key = &required_keys[0];
+
+        // Check if the key is explicitly set (either in env or config)
+        let is_set_in_env = env::var(&key.name).is_ok();
+        let is_set_in_config = config.get(&key.name, key.secret).is_ok();
+
+        return is_set_in_env || is_set_in_config;
+    }
+
+    // For providers with multiple keys or keys without defaults:
+    // Find required keys that don't have default values
+    let required_non_default_keys: Vec<&ConfigKey> = required_keys
+        .iter()
+        .filter(|key| key.default.is_none())
+        .cloned()
+        .collect();
+
+    // If there are no non-default keys, this provider needs at least one key explicitly set
+    if required_non_default_keys.is_empty() {
+        return required_keys.iter().any(|key| {
+            let is_set_in_env = env::var(&key.name).is_ok();
+            let is_set_in_config = config.get(&key.name, key.secret).is_ok();
+
+            is_set_in_env || is_set_in_config
+        });
+    }
+
+    // Otherwise, all non-default keys must be set
+    required_non_default_keys.iter().all(|key| {
+        let is_set_in_env = env::var(&key.name).is_ok();
+        let is_set_in_config = config.get(&key.name, key.secret).is_ok();
+
+        is_set_in_env || is_set_in_config
    })
+}
diff --git a/ui/desktop/src/components/settings_v2/providers/ProviderGrid.tsx b/ui/desktop/src/components/settings_v2/providers/ProviderGrid.tsx
@@ -17,23 +17,28 @@ const GridLayout = memo(function GridLayout({ children }: { children: React.Reac
 const ProviderCards = memo(function ProviderCards({
   providers,
   isOnboarding,
+  refreshProviders,
 }: {
   providers: ProviderDetails[];
   isOnboarding: boolean;
+  refreshProviders?: () => void;
 }) {
   const { openModal } = useProviderModal();
 
   // Memoize these functions so they don't get recreated on every render
   const configureProviderViaModal = useCallback(
     (provider: ProviderDetails) => {
       openModal(provider, {
-        onSubmit: (values: any) => {
-          // Your logic to save the configuration
+        onSubmit: () => {
+          // Only refresh if the function is provided
+          if (refreshProviders) {
+            refreshProviders();
+          }
         },
         formProps: {},
       });
     },
-    [openModal]
+    [openModal, refreshProviders]
   );
 
   const handleLaunch = useCallback(() => {
@@ -56,48 +61,28 @@ const ProviderCards = memo(function ProviderCards({
   return <>{providerCards}</>;
 });
 
-// Fix the ProviderModalProvider
-export const OptimizedProviderModalProvider = memo(function OptimizedProviderModalProvider({
-  children,
-}: {
-  children: React.ReactNode;
-}) {
-  const contextValue = useMemo(
-    () => ({
-      isOpen: false,
-      currentProvider: null,
-      modalProps: {},
-      openModal: (provider, additionalProps = {}) => {
-        // Implementation
-      },
-      closeModal: () => {
-        // Implementation
-      },
-    }),
-    []
-  );
-
-  return <ProviderModalProvider>{children}</ProviderModalProvider>;
-});
-
 export default memo(function ProviderGrid({
   providers,
   isOnboarding,
+  refreshProviders,
 }: {
   providers: ProviderDetails[];
   isOnboarding: boolean;
+  refreshProviders?: () => void;
 }) {
-  // Remove the console.log
-  console.log('provider grid');
   // Memoize the modal provider and its children to avoid recreating on every render
   const modalProviderContent = useMemo(
     () => (
       <ProviderModalProvider>
-        <ProviderCards providers={providers} isOnboarding={isOnboarding} />
+        <ProviderCards
+          providers={providers}
+          isOnboarding={isOnboarding}
+          refreshProviders={refreshProviders}
+        />
         <ProviderConfigurationModal />
       </ProviderModalProvider>
     ),
-    [providers, isOnboarding]
+    [providers, isOnboarding, refreshProviders]
   );
 
   return <GridLayout>{modalProviderContent}</GridLayout>;

diff --git a/ui/desktop/src/components/settings_v2/providers/ProviderSettingsPage.tsx b/ui/desktop/src/components/settings_v2/providers/ProviderSettingsPage.tsx
@@ -1,4 +1,4 @@
-import React, { useEffect, useState } from 'react';
+import React, { useEffect, useState, useCallback, useRef } from 'react';
 import { ScrollArea } from '../../ui/scroll-area';
 import BackButton from '../../ui/BackButton';
 import ProviderGrid from './ProviderGrid';
@@ -9,37 +9,40 @@ export default function ProviderSettings({ onClose }: { onClose: () => void }) {
   const { getProviders } = useConfig();
   const [loading, setLoading] = useState(true);
   const [providers, setProviders] = useState<ProviderDetails[]>([]);
+  const initialLoadDone = useRef(false);
 
-  // Load providers only once when component mounts
-  useEffect(() => {
-    let isMounted = true;
-
-    const loadProviders = async () => {
-      try {
-        // Force refresh to ensure we have the latest data
-        const result = await getProviders(true);
-        // Only update state if component is still mounted
-        if (isMounted && result) {
-          setProviders(result);
-        }
-      } catch (error) {
-        console.error('Failed to load providers:', error);
-      } finally {
-        if (isMounted) {
-          setLoading(false);
-        }
+  // Create a function to load providers that can be called multiple times
+  const loadProviders = useCallback(async () => {
+    setLoading(true);
+    try {
+      // Only force refresh when explicitly requested, not on initial load
+      const result = await getProviders(!initialLoadDone.current);
+      if (result) {
+        setProviders(result);
+        initialLoadDone.current = true;
       }
-    };
+    } catch (error) {
+      console.error('Failed to load providers:', error);
+    } finally {
+      setLoading(false);
+    }
+  }, [getProviders]);
 
+  // Load providers only once when component mounts
+  useEffect(() => {
     loadProviders();
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []); // Intentionally not including loadProviders in deps to prevent reloading
 
-    // Cleanup function to prevent state updates on unmounted component
-    return () => {
-      isMounted = false;
-    };
-  }, []); // Empty dependency array ensures this only runs once
+  // This function will be passed to ProviderGrid for manual refreshes after config changes
+  const refreshProviders = useCallback(() => {
+    if (initialLoadDone.current) {
+      getProviders(true).then((result) => {
+        if (result) setProviders(result);
+      });
+    }
+  }, [getProviders]);
 
-  console.log(providers);
   return (
     <div className="h-screen w-full">
       <div className="relative flex items-center h-[36px] w-full bg-bgSubtle"></div>
@@ -61,7 +64,11 @@ export default function ProviderSettings({ onClose }: { onClose: () => void }) {
               {loading ? (
                 <div>Loading providers...</div>
               ) : (
-                <ProviderGrid providers={providers} isOnboarding={false} />
+                <ProviderGrid
+                  providers={providers}
+                  isOnboarding={false}
+                  refreshProviders={refreshProviders}
+                />
               )}
             </div>
           </div>