diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index cf47e79f9a89..664f15a02b9f 100644 --- 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 { @@ -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 index 2adcb8b6310f..c7c68866ab27 100644 --- 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 index 83af59c23530..06a1bc3ea718 100644 --- 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)] @@ -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> { +#[allow(dead_code)] +pub fn inspect_key(key_name: &str, is_secret: bool) -> Result> { 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, Box> { @@ -89,4 +88,53 @@ pub fn inspect_keys( } Ok(results) -} \ No newline at end of file +} + +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 index a2ac34f45e90..c673f51ac012 100644 --- a/ui/desktop/src/components/settings_v2/providers/ProviderGrid.tsx +++ b/ui/desktop/src/components/settings_v2/providers/ProviderGrid.tsx @@ -17,9 +17,11 @@ 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(); @@ -27,13 +29,16 @@ const ProviderCards = memo(function ProviderCards({ 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 {children}; -}); - 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( () => ( - + ), - [providers, isOnboarding] + [providers, isOnboarding, refreshProviders] ); return {modalProviderContent}; diff --git a/ui/desktop/src/components/settings_v2/providers/ProviderSettingsPage.tsx b/ui/desktop/src/components/settings_v2/providers/ProviderSettingsPage.tsx index 1b65db7a8dc7..0c56e7a0c985 100644 --- 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([]); + 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 (
@@ -61,7 +64,11 @@ export default function ProviderSettings({ onClose }: { onClose: () => void }) { {loading ? (
Loading providers...
) : ( - + )}
diff --git a/ui/desktop/src/components/settings_v2/providers/modal/ProviderConfiguationModal.tsx b/ui/desktop/src/components/settings_v2/providers/modal/ProviderConfiguationModal.tsx index d1cb2c897b00..d2651fbef5f7 100644 --- a/ui/desktop/src/components/settings_v2/providers/modal/ProviderConfiguationModal.tsx +++ b/ui/desktop/src/components/settings_v2/providers/modal/ProviderConfiguationModal.tsx @@ -1,4 +1,4 @@ -import React, { useEffect, useState } from 'react'; +import React, { useState } from 'react'; import Modal from '../../../../components/Modal'; import ProviderSetupHeader from './subcomponents/ProviderSetupHeader'; import DefaultProviderSetupForm from './subcomponents/forms/DefaultProviderSetupForm'; @@ -20,7 +20,7 @@ const customFormsMap = { }; export default function ProviderConfigurationModal() { - const { upsert } = useConfig(); + const { upsert, getProviders } = useConfig(); const { isOpen, currentProvider, modalProps, closeModal } = useProviderModal(); const [configValues, setConfigValues] = useState({}); @@ -32,15 +32,25 @@ export default function ProviderConfigurationModal() { const SubmitHandler = customSubmitHandlerMap[currentProvider.name] || DefaultSubmitHandler; const FormComponent = customFormsMap[currentProvider.name] || DefaultProviderSetupForm; - const handleSubmitForm = (e) => { + const handleSubmitForm = async (e) => { e.preventDefault(); console.log('Form submitted for:', currentProvider.name); - SubmitHandler(upsert, currentProvider, configValues); + try { + // Wait for the submission to complete + await SubmitHandler(upsert, currentProvider, configValues); - // Close the modal unless the custom handler explicitly returns false - // This gives custom handlers the ability to keep the modal open if needed - closeModal(); + // Close the modal before triggering refreshes to avoid UI issues + closeModal(); + + // Call onSubmit callback if provided (from modal props) + if (modalProps.onSubmit) { + modalProps.onSubmit(configValues); + } + } catch (error) { + console.error('Failed to save configuration:', error); + // Keep modal open if there's an error + } }; const handleCancel = () => { diff --git a/ui/desktop/src/components/settings_v2/providers/modal/ProviderModalProvider.tsx b/ui/desktop/src/components/settings_v2/providers/modal/ProviderModalProvider.tsx index ad25186123d0..072a558733c6 100644 --- a/ui/desktop/src/components/settings_v2/providers/modal/ProviderModalProvider.tsx +++ b/ui/desktop/src/components/settings_v2/providers/modal/ProviderModalProvider.tsx @@ -1,60 +1,56 @@ -import React, { createContext, useContext, useState, useMemo, useCallback } from 'react'; -import { ProviderDetails } from '../../../../api'; +import React, { createContext, useContext, useState } from 'react'; +import { ProviderDetails } from '../../../../api/types.gen'; + +interface ModalProps { + onSubmit?: (values: any) => void; + onCancel?: () => void; + formProps?: any; +} interface ProviderModalContextType { isOpen: boolean; currentProvider: ProviderDetails | null; - modalProps: any; - openModal: (provider: ProviderDetails, additionalProps: any) => void; + modalProps: ModalProps; + openModal: (provider: ProviderDetails, additionalProps?: ModalProps) => void; closeModal: () => void; } -const defaultContext: ProviderModalContextType = { - isOpen: false, - currentProvider: null, - modalProps: {}, - openModal: () => {}, - closeModal: () => {}, -}; - -const ProviderModalContext = createContext(defaultContext); - -export const useProviderModal = () => useContext(ProviderModalContext); +const ProviderModalContext = createContext(undefined); export const ProviderModalProvider: React.FC<{ children: React.ReactNode }> = ({ children }) => { const [isOpen, setIsOpen] = useState(false); const [currentProvider, setCurrentProvider] = useState(null); - const [modalProps, setModalProps] = useState({}); + const [modalProps, setModalProps] = useState({}); - // Use useCallback to prevent function recreation on each render - const openModal = useCallback((provider: ProviderDetails, additionalProps = {}) => { + const openModal = (provider: ProviderDetails, additionalProps: ModalProps = {}) => { setCurrentProvider(provider); setModalProps(additionalProps); setIsOpen(true); - }, []); + }; - const closeModal = useCallback(() => { + const closeModal = () => { setIsOpen(false); - // Use a small timeout to prevent UI flicker - setTimeout(() => { - setCurrentProvider(null); - setModalProps({}); - }, 200); - }, []); - - // Memoize the context value to prevent unnecessary re-renders - const contextValue = useMemo( - () => ({ - isOpen, - currentProvider, - modalProps, - openModal, - closeModal, - }), - [isOpen, currentProvider, modalProps, openModal, closeModal] - ); + }; return ( - {children} + + {children} + ); }; + +export const useProviderModal = () => { + const context = useContext(ProviderModalContext); + if (context === undefined) { + throw new Error('useProviderModal must be used within a ProviderModalProvider'); + } + return context; +}; diff --git a/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx b/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx index 9ab49509b562..f9af1abcb3d9 100644 --- a/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx +++ b/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx @@ -25,12 +25,11 @@ export default function DefaultProviderSetupForm({ // Try to load actual values from config for each parameter that is not secret for (const parameter of parameters) { - if (parameter.required && !parameter.secret) { + if (parameter.required) { try { // Check if there's a stored value in the config system const configKey = `${parameter.name}`; const configResponse = await read(configKey, parameter.secret || false); - console.log('configResponse', configResponse); if (configResponse) { // Use the value from the config provider diff --git a/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx b/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx index 472fc597373b..94a6a2d3181f 100644 --- a/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx +++ b/ui/desktop/src/components/settings_v2/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx @@ -101,8 +101,7 @@ export const DefaultSubmitHandler = async (upsertFn, provider, configValues) => // Create the provider-specific config key const configKey = `${parameter.name}`; - // Explicitly define is_secret as a boolean (true/false) or null - // This is critical for Rust's Option type + // Explicitly define is_secret as a boolean (true/false) const isSecret = parameter.secret === true; // Pass the is_secret flag from the parameter definition