From 30d40a6c2899c0f84773ff36f39386eb7ffbac4b Mon Sep 17 00:00:00 2001 From: Carine Bruyndoncx Date: Mon, 8 Sep 2025 12:30:53 +0200 Subject: [PATCH] Fixes and improvements to get custom providers working in the UI, and improving the providers edit box exposing more fields, and the grid for UX experience, similar to extensions Signed-off-by: Carine Bruyndoncx --- .../src/routes/config_management.rs | 208 +++++++--- crates/goose-server/src/routes/reply.rs | 37 +- crates/goose/src/config/custom_providers.rs | 59 +++ crates/goose/src/providers/api_client.rs | 85 +++- crates/goose/src/providers/formats/openai.rs | 71 +++- crates/goose/src/providers/openai.rs | 4 +- .../goose/src/providers/provider_registry.rs | 3 +- crates/goose/src/providers/utils.rs | 39 +- .../settings/providers/ProviderGrid.tsx | 237 +++++------ .../modal/ProviderConfiguationModal.tsx | 389 +++++++++++++++++- .../forms/CustomProviderForm.tsx | 2 +- .../forms/DefaultProviderSetupForm.tsx | 166 +++++--- .../handlers/DefaultSubmitHandler.tsx | 8 + ui/desktop/src/utils/secretConstants.ts | 1 + 14 files changed, 1050 insertions(+), 259 deletions(-) create mode 100644 ui/desktop/src/utils/secretConstants.ts diff --git a/crates/goose-server/src/routes/config_management.rs b/crates/goose-server/src/routes/config_management.rs index 5c5b8bbb2f47..df3aee4ff0a1 100644 --- a/crates/goose-server/src/routes/config_management.rs +++ b/crates/goose-server/src/routes/config_management.rs @@ -3,7 +3,7 @@ use crate::routes::utils::check_provider_configured; use crate::state::AppState; use axum::{ extract::{Path, State}, - routing::{delete, get, post}, + routing::{delete, get, post, put}, Json, Router, }; use etcetera::{choose_app_strategy, AppStrategy}; @@ -87,6 +87,19 @@ pub struct CreateCustomProviderRequest { pub supports_streaming: Option, } +#[derive(Deserialize, Serialize, ToSchema)] +pub struct UpdateCustomProviderRequest { + pub display_name: Option, + pub api_url: Option, + pub api_key: Option, + pub models: Option>, + pub supports_streaming: Option, + // Editable provider JSON fields + pub description: Option, + pub headers: Option>, + pub timeout_seconds: Option, +} + #[utoipa::path( post, path = "/config/upsert", @@ -104,6 +117,18 @@ pub async fn upsert_config( verify_secret_key(&headers, &state)?; let config = Config::global(); + + // Defensive guard: if this is a secret check and the client sent a boolean 'true' to + // indicate presence, do not write that boolean into the secret storage. Treat as a no-op. + if query.is_secret { + if let Value::Bool(b) = &query.value { + if *b { + tracing::info!(key = %query.key, "Skipping upsert for secret key with boolean true (presence-only)"); + return Ok(Json(Value::String(format!("Skipped secret upsert for {} (presence-only)", query.key)))); + } + } + } + let result = config.set(&query.key, query.value, query.is_secret); match result { @@ -316,64 +341,7 @@ pub async fn providers( ) -> Result>, StatusCode> { verify_secret_key(&headers, &state)?; - let mut providers_metadata = get_providers(); - - let custom_providers_dir = goose::config::custom_providers::custom_providers_dir(); - - if custom_providers_dir.exists() { - if let Ok(entries) = std::fs::read_dir(&custom_providers_dir) { - for entry in entries.flatten() { - if let Some(extension) = entry.path().extension() { - if extension == "json" { - if let Ok(content) = std::fs::read_to_string(entry.path()) { - if let Ok(custom_provider) = serde_json::from_str::< - goose::config::custom_providers::CustomProviderConfig, - >(&content) - { - // CustomProviderConfig => ProviderMetadata - let default_model = custom_provider - .models - .first() - .map(|m| m.name.clone()) - .unwrap_or_default(); - - let metadata = goose::providers::base::ProviderMetadata { - name: custom_provider.name.clone(), - display_name: custom_provider.display_name.clone(), - description: custom_provider - .description - .clone() - .unwrap_or_else(|| { - format!("{} (custom)", custom_provider.display_name) - }), - default_model, - known_models: custom_provider.models.clone(), - model_doc_link: "Custom provider".to_string(), - config_keys: vec![ - goose::providers::base::ConfigKey::new( - &custom_provider.api_key_env, - true, - true, - None, - ), - goose::providers::base::ConfigKey::new( - "CUSTOM_PROVIDER_BASE_URL", - true, - false, - Some(&custom_provider.base_url), - ), - ], - }; - providers_metadata.push(metadata); - } - } - } - } - } - } - } - - let providers_response: Vec = providers_metadata + let providers_response: Vec = get_providers() .into_iter() .map(|metadata| { let is_configured = check_provider_configured(&metadata); @@ -766,6 +734,8 @@ pub async fn get_current_model( #[utoipa::path( post, + + path = "/config/custom-providers", request_body = CreateCustomProviderRequest, responses( @@ -798,6 +768,124 @@ pub async fn create_custom_provider( Ok(Json(format!("Custom provider added - ID: {}", config.id()))) } +#[utoipa::path( + put, + path = "/config/custom-providers/{id}", + request_body = UpdateCustomProviderRequest, + responses( + (status = 200, description = "Custom provider updated successfully", body = String), + (status = 400, description = "Invalid request"), + (status = 404, description = "Provider not found"), + (status = 500, description = "Internal server error") + ) +)] +pub async fn update_custom_provider( + State(state): State>, + headers: HeaderMap, + axum::extract::Path(id): axum::extract::Path, + Json(request): Json, +) -> Result, StatusCode> { + verify_secret_key(&headers, &state)?; + + // Log incoming update for debugging + let payload_value = serde_json::to_value(&request).unwrap_or(serde_json::Value::Null); + tracing::info!(id = %id, payload = ?payload_value, "update_custom_provider called"); + + let custom_providers_dir = goose::config::custom_providers::custom_providers_dir(); + let file_path = custom_providers_dir.join(format!("{}.json", id)); + + if !file_path.exists() { + return Err(StatusCode::NOT_FOUND); + } + + // Read existing config + let content = std::fs::read_to_string(&file_path).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + let mut config: goose::config::custom_providers::CustomProviderConfig = + serde_json::from_str(&content).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + + // Update fields if provided + if let Some(display_name) = request.display_name { + config.display_name = display_name; + } + if let Some(api_url) = request.api_url { + config.base_url = api_url; + } + if let Some(models) = request.models { + config.models = models + .into_iter() + .map(|m| goose::providers::base::ModelInfo::new(m, 128000)) + .collect(); + } + if let Some(s) = request.supports_streaming { + config.supports_streaming = Some(s); + } + + // Persist API key into secrets if provided + // Update optional JSON fields if provided + if let Some(desc) = request.description { + config.description = Some(desc); + } + if let Some(hdrs) = request.headers { + config.headers = Some(hdrs); + } + if let Some(t) = request.timeout_seconds { + config.timeout_seconds = Some(t); + } + + // Persist API key into secrets if provided + if let Some(api_key) = request.api_key { + let cfg = goose::config::Config::global(); + if let Err(e) = cfg.set_secret(&config.api_key_env, serde_json::Value::String(api_key)) { + tracing::error!("Failed to set secret for {}: {}", config.api_key_env, e); + return Err(StatusCode::INTERNAL_SERVER_ERROR); + } + } + + // Save updated JSON atomically + let tmp = file_path.with_extension("json.tmp"); + let json_content = serde_json::to_string_pretty(&config).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + std::fs::write(&tmp, &json_content).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + std::fs::rename(&tmp, &file_path).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + + // Refresh in-memory providers + if let Err(e) = goose::providers::refresh_custom_providers() { + tracing::warn!("Failed to refresh custom providers after update: {}", e); + } + + Ok(Json(format!("Updated custom provider: {}", id))) +} + + +#[utoipa::path( + get, + path = "/config/custom-providers/{id}", + responses( + (status = 200, description = "Custom provider retrieved successfully", body = goose::config::custom_providers::CustomProviderConfig), + (status = 404, description = "Provider not found"), + (status = 500, description = "Internal server error") + ) +)] +pub async fn get_custom_provider( + State(state): State>, + headers: HeaderMap, + axum::extract::Path(id): axum::extract::Path, +) -> Result, StatusCode> { + verify_secret_key(&headers, &state)?; + + let custom_providers_dir = goose::config::custom_providers::custom_providers_dir(); + let file_path = custom_providers_dir.join(format!("{}.json", id)); + + if !file_path.exists() { + return Err(StatusCode::NOT_FOUND); + } + + let content = std::fs::read_to_string(&file_path).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + let config: goose::config::custom_providers::CustomProviderConfig = + serde_json::from_str(&content).map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + + Ok(Json(config)) +} + #[utoipa::path( delete, path = "/config/custom-providers/{id}", @@ -845,7 +933,7 @@ pub fn routes(state: Arc) -> Router { .route("/config/custom-providers", post(create_custom_provider)) .route( "/config/custom-providers/{id}", - delete(remove_custom_provider), + get(get_custom_provider).delete(remove_custom_provider).put(update_custom_provider), ) .with_state(state) } diff --git a/crates/goose-server/src/routes/reply.rs b/crates/goose-server/src/routes/reply.rs index a11ce3134501..0af8a727b859 100644 --- a/crates/goose-server/src/routes/reply.rs +++ b/crates/goose-server/src/routes/reply.rs @@ -265,14 +265,17 @@ async fn reply_handler( Ok(stream) => stream, Err(e) => { tracing::error!("Failed to start reply stream: {:?}", e); - stream_event( - MessageEvent::Error { - error: e.to_string(), - }, + let err_text = e.to_string(); + // send Error event (for telemetry / UI error handling) + let _ = stream_event( + MessageEvent::Error { error: err_text.clone() }, &task_tx, &cancel_token, ) .await; + // also send a visible assistant message so the UI shows it inline + let assistant_msg = Message::assistant().with_text(format!("Provider error: {}", err_text)); + let _ = stream_event(MessageEvent::Message { message: assistant_msg }, &task_tx, &cancel_token).await; return; } }; @@ -312,8 +315,32 @@ async fn reply_handler( track_tool_telemetry(content, all_messages.messages()); } + // Push and send the message event all_messages.push(message.clone()); - stream_event(MessageEvent::Message { message }, &tx, &cancel_token).await; + stream_event(MessageEvent::Message { message: message.clone() }, &tx, &cancel_token).await; + + // If this message appears to be a provider streaming error produced by + // the OpenAI-format streaming handler, surface an Error event as + // well so the frontend's error handling path runs and the UI + // visibly shows the failure. We detect this by looking for the + // distinctive prefix we emit when streaming errors are encountered. + if let Some(first_text) = message + .content + .iter() + .find_map(|c| match c { + goose::conversation::message::MessageContent::Text(t) => Some(t.text.clone()), + _ => None, + }) + .filter(|s| s.starts_with("LLM streaming error encountered")) + { + // Send a short error string (avoid flooding the SSE with huge payloads) + let short = if first_text.len() > 1024 { + format!("{}...", &first_text[..1024]) + } else { + first_text + }; + let _ = stream_event(MessageEvent::Error { error: short }, &tx, &cancel_token).await; + } } Ok(Some(Ok(AgentEvent::HistoryReplaced(new_messages)))) => { // Replace the message history with the compacted messages diff --git a/crates/goose/src/config/custom_providers.rs b/crates/goose/src/config/custom_providers.rs index 3110771a185e..250a78b3c7e0 100644 --- a/crates/goose/src/config/custom_providers.rs +++ b/crates/goose/src/config/custom_providers.rs @@ -148,8 +148,37 @@ pub fn register_custom_providers( ) -> Result<()> { let configs = load_custom_providers(dir)?; + // Detect legacy shared key usage in keyring/config that could override + // per-provider base URLs. Historically a single key name + // "CUSTOM_PROVIDER_BASE_URL" was used for all providers which could + // result in cross-provider collisions if stored in the keyring. If that + // legacy key exists, log a warning so operators can remove/migrate it. + let global_config = crate::config::Config::global(); + if let Ok(val) = global_config.get_secret::("CUSTOM_PROVIDER_BASE_URL") { + tracing::warn!("Detected legacy shared key 'CUSTOM_PROVIDER_BASE_URL' in secret storage. This can cause custom providers to use the wrong base_url. Value: {}.", val); + // Attempt to remove the legacy shared key from secret storage. This is + // a best-effort cleanup to prevent custom providers from picking up a + // wrong global base_url. If deletion fails, log the error and continue + // without panicking. + match global_config.delete_secret("CUSTOM_PROVIDER_BASE_URL") { + Ok(_) => tracing::info!( + "Removed legacy secret key 'CUSTOM_PROVIDER_BASE_URL' from secret storage." + ), + Err(e) => tracing::error!( + "Failed to remove legacy secret key 'CUSTOM_PROVIDER_BASE_URL': {}", + e + ), + } + } + for config in configs { let config_clone = config.clone(); + // Use a unique base URL key per custom provider to avoid collisions in the + // global config/keyring. Previously this used the constant + // "CUSTOM_PROVIDER_BASE_URL" for every provider which caused different + // providers to read/write the same key and mix up values stored in the + // keyring or config file. + let base_url_key = format!("{}_BASE_URL", config.name.to_uppercase()); let description = config .description .clone() @@ -174,36 +203,66 @@ pub fn register_custom_providers( match config.engine { ProviderEngine::OpenAI => { + let config_keys = vec![ + crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None), + crate::providers::base::ConfigKey::new( + &base_url_key, + true, + false, + Some(&config.base_url), + ), + ]; registry.register_with_name::( config.name.clone(), config.display_name.clone(), description, default_model, known_models, + config_keys, move |model: ModelConfig| { OpenAiProvider::from_custom_config(model, config_clone.clone()) }, ); } ProviderEngine::Ollama => { + let config_keys = vec![ + crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None), + crate::providers::base::ConfigKey::new( + &base_url_key, + true, + false, + Some(&config.base_url), + ), + ]; registry.register_with_name::( config.name.clone(), config.display_name.clone(), description, default_model, known_models, + config_keys, move |model: ModelConfig| { OllamaProvider::from_custom_config(model, config_clone.clone()) }, ); } ProviderEngine::Anthropic => { + let config_keys = vec![ + crate::providers::base::ConfigKey::new(&config.api_key_env, true, true, None), + crate::providers::base::ConfigKey::new( + &base_url_key, + true, + false, + Some(&config.base_url), + ), + ]; registry.register_with_name::( config.name.clone(), config.display_name.clone(), description, default_model, known_models, + config_keys, move |model: ModelConfig| { AnthropicProvider::from_custom_config(model, config_clone.clone()) }, diff --git a/crates/goose/src/providers/api_client.rs b/crates/goose/src/providers/api_client.rs index 821148bae757..20c5f06b8654 100644 --- a/crates/goose/src/providers/api_client.rs +++ b/crates/goose/src/providers/api_client.rs @@ -184,9 +184,39 @@ impl fmt::Debug for AuthMethod { } impl ApiResponse { - pub async fn from_response(response: Response) -> Result { + /// Parse a `reqwest::Response` into an ApiResponse. Optionally accepts the + /// original request payload (as JSON string) and a provider hint so we can + /// log all three (request payload, URL and response body) when an error + /// occurs. This helps debugging when the desktop auto-starts the server + /// and you cannot enable debug logging easily. + pub async fn from_response( + response: Response, + request_payload: Option<&str>, + provider_hint: Option<&str>, + ) -> Result { let status = response.status(); - let payload = response.json().await.ok(); + + // Capture URL and raw body text for debugging when there is an error + let url = response.url().clone(); + let response = response; + let text_body = response.text().await.ok(); + + if !status.is_success() { + tracing::error!( + %url, + status = %status, + provider = %provider_hint.unwrap_or(""), + request = %request_payload.unwrap_or(""), + response = %text_body.as_deref().unwrap_or(""), + "LLM_RESPONSE_ERROR" + ); + } + + // If the body is valid JSON, parse it into payload; otherwise use None + let payload = text_body + .as_deref() + .and_then(|t| serde_json::from_str::(t).ok()); + Ok(Self { status, payload }) } } @@ -305,9 +335,20 @@ impl ApiClient { base_url.set_path(&format!("{}/", base_path)); } - base_url + // Join the requested path onto the base URL. + let mut joined = base_url .join(path) - .map_err(|e| anyhow::anyhow!("Failed to construct URL: {}", e)) + .map_err(|e| anyhow::anyhow!("Failed to construct URL: {}", e))?; + + // If the caller's path does not explicitly end with a slash, ensure we + // do not introduce a trailing slash on the final URL. Some remote APIs + // treat a trailing slash as a different endpoint and return 404. + if !path.ends_with('/') { + let url_str = joined.as_str().trim_end_matches('/').to_string(); + joined = Url::parse(&url_str).map_err(|e| anyhow::anyhow!("Failed to normalize URL: {}", e))?; + } + + Ok(joined) } async fn get_oauth_token(&self, config: &OAuthConfig) -> Result { @@ -336,24 +377,40 @@ impl<'a> ApiRequestBuilder<'a> { } pub async fn api_post(self, payload: &Value) -> Result { + let payload_str = serde_json::to_string(payload).unwrap_or_else(|_| "{}".to_string()); + let provider_hint = self.client.host.as_str(); let response = self.response_post(payload).await?; - ApiResponse::from_response(response).await + ApiResponse::from_response(response, Some(&payload_str), Some(provider_hint)).await } pub async fn response_post(self, payload: &Value) -> Result { - // Log the JSON payload being sent to the LLM - tracing::debug!( - "LLM_REQUEST: {}", - serde_json::to_string(payload).unwrap_or_else(|_| "{}".to_string()) - ); - - let request = self.send_request(|url, client| client.post(url)).await?; - Ok(request.json(payload).send().await?) + // Log the JSON payload being sent to the LLM and the final URL being called + let payload_str = serde_json::to_string(payload).unwrap_or_else(|_| "{}".to_string()); + tracing::debug!(payload = %payload_str, path = %self.path, "LLM_REQUEST"); + + let request_builder = self.send_request(|url, client| client.post(url)).await?; + // For additional visibility, attempt to extract the final URL from the builder + // Note: reqwest RequestBuilder does not expose final URL before send, so we build the + // request to get the URL for logging. This consumes the builder into a Request. + match request_builder.try_clone() { + Some(rb) => { + if let Ok(req) = rb.json(payload).build() { + tracing::debug!(url = %req.url(), "LLM_OUTGOING_URL"); + } + } + None => { + tracing::debug!(path = %self.path, "LLM_OUTGOING_PATH_NO_CLONE"); + } + } + + Ok(request_builder.json(payload).send().await?) } pub async fn api_get(self) -> Result { + // GET doesn't always have a payload, but keep consistency + let provider_hint = self.client.host.as_str(); let response = self.response_get().await?; - ApiResponse::from_response(response).await + ApiResponse::from_response(response, None, Some(provider_hint)).await } pub async fn response_get(self) -> Result { diff --git a/crates/goose/src/providers/formats/openai.rs b/crates/goose/src/providers/formats/openai.rs index 3ff4b712a867..b3f11a2d7e1e 100644 --- a/crates/goose/src/providers/formats/openai.rs +++ b/crates/goose/src/providers/formats/openai.rs @@ -36,6 +36,10 @@ struct Delta { content: Option, role: Option, tool_calls: Option>, + // Some providers put a 'name' (tool name) here instead of in tool_calls + name: Option, + // OpenAI/variants may include a 'refusal' field with refusal details + refusal: Option, } #[derive(Serialize, Deserialize, Debug)] @@ -413,7 +417,20 @@ fn ensure_valid_json_schema(schema: &mut Value) { } fn strip_data_prefix(line: &str) -> Option<&str> { - line.strip_prefix("data: ").map(|s| s.trim()) + let s = line.trim(); + // SSE style: "data: {json}" + if let Some(rest) = s.strip_prefix("data: ") { + return Some(rest.trim()); + } + // Raw NDJSON/JSON per-line: "{...}" or "[...]" + if s.starts_with('{') || s.starts_with('[') { + return Some(s); + } + // Some providers send the done sentinel without the data: prefix + if s == "[DONE]" { + return Some(s); + } + None } pub fn response_to_streaming_message( @@ -426,19 +443,63 @@ where use futures::StreamExt; 'outer: while let Some(response) = stream.next().await { - if response.as_ref().is_ok_and(|s| s == "data: [DONE]") { + // Accept both SSE style "data: [DONE]" and plain "[DONE]" sentinel + if response.as_ref().is_ok_and(|s| s.trim() == "data: [DONE]" || s.trim() == "[DONE]") { break 'outer; } let response_str = response?; + tracing::debug!(raw_stream_line = %response_str, "LLM_STREAM_RAW_LINE"); let line = strip_data_prefix(&response_str); if line.is_none() || line.is_some_and(|l| l.is_empty()) { continue } - let chunk: StreamingChunk = serde_json::from_str(line - .ok_or_else(|| anyhow!("unexpected stream format"))?) - .map_err(|e| anyhow!("Failed to parse streaming chunk: {}: {:?}", e, &line))?; + let line_str = line + .ok_or_else(|| anyhow!("unexpected stream format"))?; + + // First parse into a generic JSON value so we can detect provider-specific + // error objects (some OpenAI-compatible endpoints emit {"object":"error",...}). + let v: Value = serde_json::from_str(line_str) + .map_err(|e| anyhow!("Failed to parse streaming JSON value: {}: {:?}", e, &line_str))?; + + // Detect top-level error objects and yield a structured error message so the UI + // can render provider errors consistently during streaming. Previously we returned + // Err(...) which terminated the stream without providing a message object to the + // frontend; that made debugging custom providers difficult because the UI had no + // provider response to display. + if v.get("object").and_then(|o| o.as_str()).map(|s| s == "error").unwrap_or(false) + || v.get("error").is_some() + { + // Build a helpful text representation including status-like fields when available + let mut details = String::new(); + // Attempt to include an explicit "message" field if present + if let Some(m) = v.get("message") { + details.push_str(&format!("message: {}\n", m)); + } + // Include any nested error object + if let Some(err_obj) = v.get("error") { + details.push_str(&format!("error: {}\n", err_obj)); + } + // Include the full JSON payload as a last resort + details.push_str(&format!("raw: {}", v)); + + // Construct a Message that contains the error details so the UI can display it. + // Use assistant role so it is shown where other assistant output appears, but + // include the error text so renderers can style it as an error. + let msg_text = format!("LLM streaming error encountered. See details below:\n{}", details); + + let message = Message::assistant().with_content(MessageContent::text(msg_text)); + + // Yield the error message along with any usage info collected so far. + yield (Some(message), None); + + // After yielding the error message, stop processing the stream. + break 'outer; + } + + let chunk: StreamingChunk = serde_json::from_value(v.clone()) + .map_err(|e| anyhow!("Failed to parse streaming chunk: {}: {:?}", e, &line_str))?; let usage = chunk.usage.as_ref().and_then(|u| { chunk.model.as_ref().map(|model| { diff --git a/crates/goose/src/providers/openai.rs b/crates/goose/src/providers/openai.rs index 3f493dcd8f6e..788159d58cb2 100644 --- a/crates/goose/src/providers/openai.rs +++ b/crates/goose/src/providers/openai.rs @@ -278,10 +278,12 @@ impl Provider for OpenAiProvider { let mut payload = create_request(&self.model, system, messages, tools, &ImageFormat::OpenAi)?; payload["stream"] = serde_json::Value::Bool(true); + + /* TODO Carine Bruyndoncx Disable for hyperbolic test */ payload["stream_options"] = json!({ "include_usage": true, }); - + let response = self .api_client .response_post(&self.base_path, &payload) diff --git a/crates/goose/src/providers/provider_registry.rs b/crates/goose/src/providers/provider_registry.rs index e20bc98dd106..d8dd6b0bf57c 100644 --- a/crates/goose/src/providers/provider_registry.rs +++ b/crates/goose/src/providers/provider_registry.rs @@ -48,6 +48,7 @@ impl ProviderRegistry { description: String, default_model: String, known_models: Vec, + config_keys: Vec, constructor: F, ) where P: Provider + 'static, @@ -61,7 +62,7 @@ impl ProviderRegistry { default_model, known_models, model_doc_link: base_metadata.model_doc_link, - config_keys: base_metadata.config_keys, + config_keys, }; self.entries.insert( diff --git a/crates/goose/src/providers/utils.rs b/crates/goose/src/providers/utils.rs index fd1766ff515b..40f0754eed8d 100644 --- a/crates/goose/src/providers/utils.rs +++ b/crates/goose/src/providers/utils.rs @@ -86,12 +86,32 @@ pub fn map_http_error_to_provider_error( if check_context_length_exceeded(&payload_str) { ProviderError::ContextLengthExceeded(payload_str) } else { + // Try multiple ways to extract a useful message + // 1. OpenAI style: {"error": {"message": "..."}} if let Some(error) = payload.get("error") { - error_msg = error - .get("message") - .and_then(|m| m.as_str()) - .unwrap_or("Unknown error") - .to_string(); + if let Some(m) = error.get("message").and_then(|m| m.as_str()) { + error_msg = m.to_string(); + } else if let Some(m) = error.as_str() { + error_msg = m.to_string(); + } + } + // 2. Top-level "message" field + if error_msg == "Unknown error" { + if let Some(m) = payload.get("message").and_then(|m| m.as_str()) { + error_msg = m.to_string(); + } + } + // 3. Other common fields + if error_msg == "Unknown error" { + if let Some(m) = payload.get("error_message").and_then(|m| m.as_str()) { + error_msg = m.to_string(); + } else if let Some(m) = payload.get("detail").and_then(|m| m.as_str()) { + error_msg = m.to_string(); + } + } + // 4. If still unknown, include the whole payload for debugging + if error_msg == "Unknown error" { + error_msg = payload_str; } ProviderError::RequestFailed(format!( "Request failed with status: {}. Message: {}", @@ -166,6 +186,15 @@ pub async fn handle_status_openai_compat(response: Response) -> Result(&body_str).ok(); + // attempt to parse top-level message fields for non-OpenAI error shapes + if let Ok(payload_json) = serde_json::from_str::(&body_str) { + if let Some(m) = payload_json.get("message").and_then(|m| m.as_str()) { + // Surface top-level "message" errors (some providers use this shape) + return Err(ProviderError::RequestFailed(format!("{} (status {})", m, status.as_u16()))); + } + } + + Err(map_http_error_to_provider_error(status, payload)) } diff --git a/ui/desktop/src/components/settings/providers/ProviderGrid.tsx b/ui/desktop/src/components/settings/providers/ProviderGrid.tsx index 661342782ae9..e01856bb5263 100644 --- a/ui/desktop/src/components/settings/providers/ProviderGrid.tsx +++ b/ui/desktop/src/components/settings/providers/ProviderGrid.tsx @@ -43,8 +43,7 @@ const CustomProviderCard = memo(function CustomProviderCard({ onClick }: { onCli ); }); -// Memoize the ProviderCards component -const ProviderCards = memo(function ProviderCards({ +export default memo(function ProviderGrid({ providers, isOnboarding, refreshProviders, @@ -53,135 +52,143 @@ const ProviderCards = memo(function ProviderCards({ providers: ProviderDetails[]; isOnboarding: boolean; refreshProviders?: () => void; - onProviderLaunch: (provider: ProviderDetails) => void; + onProviderLaunch?: (provider: ProviderDetails) => void; }) { - const { openModal } = useProviderModal(); - const [showCustomProviderModal, setShowCustomProviderModal] = useState(false); - - // Memoize these functions so they don't get recreated on every render - const configureProviderViaModal = useCallback( - (provider: ProviderDetails) => { - openModal(provider, { - onSubmit: () => { - // Only refresh if the function is provided - if (refreshProviders) { - refreshProviders(); - } - }, - onDelete: (_values: unknown) => { - if (refreshProviders) { - refreshProviders(); - } - }, - formProps: {}, - }); - }, - [openModal, refreshProviders] - ); + const launch = onProviderLaunch || (() => {}); - const deleteProviderConfigViaModal = useCallback( - (provider: ProviderDetails) => { - openModal(provider, { - onDelete: (_values: unknown) => { - // Only refresh if the function is provided - if (refreshProviders) { - refreshProviders(); - } - }, - formProps: {}, - }); - }, - [openModal, refreshProviders] - ); + // Inner component that uses the provider modal context + const ProvidersContent = memo(function ProvidersContent() { + const [showCustomProviderModal, setShowCustomProviderModal] = useState(false); + const { openModal } = useProviderModal(); + + const configureProviderViaModal = useCallback( + (provider: ProviderDetails) => { + openModal(provider, { + onSubmit: async () => { + if (refreshProviders) await refreshProviders(); + }, + onDelete: (_values: unknown) => { + if (refreshProviders) refreshProviders(); + }, + formProps: {}, + }); + }, + [openModal] + ); + + const deleteProviderConfigViaModal = useCallback( + (provider: ProviderDetails) => { + openModal(provider, { + onDelete: (_values: unknown) => { + if (refreshProviders) refreshProviders(); + }, + formProps: {}, + }); + }, + [openModal] + ); - const handleCreateCustomProvider = useCallback( - async (data: CreateCustomProviderRequest) => { + const handleCreateCustomProvider = useCallback(async (data: CreateCustomProviderRequest) => { try { const { createCustomProvider } = await import('../../../api'); await createCustomProvider({ body: data }); setShowCustomProviderModal(false); - if (refreshProviders) { - refreshProviders(); - } + if (refreshProviders) await refreshProviders(); } catch (error) { console.error('Failed to create custom provider:', error); } - }, - [refreshProviders] - ); + }, []); - // Use useMemo to memoize the cards array - const providerCards = useMemo(() => { - // providers needs to be an array - const providersArray = Array.isArray(providers) ? providers : []; - const cards = providersArray.map((provider) => ( - configureProviderViaModal(provider)} - onDelete={() => deleteProviderConfigViaModal(provider)} - onLaunch={() => onProviderLaunch(provider)} - isOnboarding={isOnboarding} - /> - )); - - cards.push( - setShowCustomProviderModal(true)} /> - ); + const providerCardsByGroup = useMemo(() => { + // Copy the array before sorting to avoid mutating props + const providersArray = Array.isArray(providers) ? providers.slice() : []; - return cards; - }, [ - providers, - isOnboarding, - configureProviderViaModal, - deleteProviderConfigViaModal, - onProviderLaunch, - ]); + // Split into configured and available, then sort each group alphabetically by provider name + const configured = providersArray + .filter((p) => p.is_configured) + .sort((a, b) => + (a.metadata?.display_name || a.name).localeCompare(b.metadata?.display_name || b.name) + ); - return ( - <> - {providerCards} - - - - - Add Custom Provider - - setShowCustomProviderModal(false)} - /> - - - - ); -}); + const available = providersArray + .filter((p) => !p.is_configured) + .sort((a, b) => + (a.metadata?.display_name || a.name).localeCompare(b.metadata?.display_name || b.name) + ); -export default memo(function ProviderGrid({ - providers, - isOnboarding, - refreshProviders, - onProviderLaunch, -}: { - providers: ProviderDetails[]; - isOnboarding: boolean; - refreshProviders?: () => void; - onProviderLaunch?: (provider: ProviderDetails) => void; -}) { - // Memoize the modal provider and its children to avoid recreating on every render - const modalProviderContent = useMemo( - () => ( - - ( + configureProviderViaModal(provider)} + onDelete={() => deleteProviderConfigViaModal(provider)} + onLaunch={() => launch(provider)} isOnboarding={isOnboarding} - refreshProviders={refreshProviders} - onProviderLaunch={onProviderLaunch || (() => {})} /> + )); + + const availableCards = available.map((provider) => ( + configureProviderViaModal(provider)} + onDelete={() => deleteProviderConfigViaModal(provider)} + onLaunch={() => launch(provider)} + isOnboarding={isOnboarding} + /> + )); + + return { configuredCards, availableCards }; + }, [configureProviderViaModal, deleteProviderConfigViaModal]); + + return ( +
+ {providerCardsByGroup.configuredCards.length > 0 && ( +
+

+ + Configured Providers ({providerCardsByGroup.configuredCards.length}) +

+ + {providerCardsByGroup.configuredCards} + setShowCustomProviderModal(true)} + /> + +
+ )} + + {providerCardsByGroup.availableCards.length > 0 && ( +
+

+ + Available Providers ({providerCardsByGroup.availableCards.length}) +

+ {providerCardsByGroup.availableCards} +
+ )} + - - ), - [providers, isOnboarding, refreshProviders, onProviderLaunch] + + + + + Add Custom Provider + + setShowCustomProviderModal(false)} + /> + + +
+ ); + }); + + return ( + + + ); - return {modalProviderContent}; }); diff --git a/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx b/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx index 58b0df67dc86..c48fa2b20348 100644 --- a/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx +++ b/ui/desktop/src/components/settings/providers/modal/ProviderConfiguationModal.tsx @@ -1,4 +1,6 @@ import { useEffect, useState } from 'react'; +import { SECRET_PRESENT_SENTINEL } from '../../../../utils/secretConstants'; + import { Dialog, DialogContent, @@ -18,6 +20,7 @@ import OllamaForm from './subcomponents/forms/OllamaForm'; import { useConfig } from '../../../ConfigContext'; import { useModelAndProvider } from '../../../ModelAndProviderContext'; import { AlertTriangle } from 'lucide-react'; +import { toast } from 'react-toastify'; import { ConfigKey, removeCustomProvider } from '../../../../api'; interface FormValues { @@ -34,7 +37,7 @@ const customFormsMap: Record = { export default function ProviderConfigurationModal() { const [validationErrors, setValidationErrors] = useState>({}); - const { upsert, remove } = useConfig(); + const { upsert, remove, getProviders } = useConfig(); const { getCurrentModelAndProvider } = useModelAndProvider(); const { isOpen, currentProvider, modalProps, closeModal } = useProviderModal(); const [configValues, setConfigValues] = useState>({}); @@ -53,6 +56,95 @@ export default function ProviderConfigurationModal() { setValidationErrors({}); setShowDeleteConfirmation(false); setIsActiveProvider(false); // Reset active provider state + + // If this is a custom provider, fetch the full provider JSON to pre-fill extra fields + if (currentProvider.name.startsWith('custom_')) { + (async () => { + try { + const secretKey = await window.electron.getSecretKey(); + // Try generated API client first + try { + /* eslint-disable @typescript-eslint/no-explicit-any */ + const clientMod = await import('../../../../api/client.gen'); + const client = ( + clientMod as unknown as { client?: { get?: (...args: any[]) => Promise } } + ).client; + if (client && typeof client.get === 'function') { + const resp = await client.get({ + url: '/config/custom-providers/{id}', + path: { id: currentProvider.name }, + headers: { 'X-Secret-Key': secretKey }, + }); + const body = + (resp as unknown as any)?.data ?? (resp as unknown as any)?.body ?? resp; + if (body) { + setConfigValues((prev) => ({ + ...prev, + display_name: body.display_name ?? prev.display_name, + description: body.description ?? prev.description, + headers: body.headers ? JSON.stringify(body.headers) : prev.headers, + timeout_seconds: body.timeout_seconds + ? String(body.timeout_seconds) + : prev.timeout_seconds, + supports_streaming: + body.supports_streaming !== undefined + ? body.supports_streaming + : prev.supports_streaming, + })); + } + return; + } + /* eslint-enable @typescript-eslint/no-explicit-any */ + } catch (e) { + console.debug('API client get failed, falling back to fetch', e); + } + + // Fallback to a direct fetch + try { + const electronCfg = + window.electron && window.electron.getConfig ? window.electron.getConfig() : null; + const hostCfg = electronCfg?.GOOSE_API_HOST ?? window.appConfig?.get('GOSE_API_HOST'); + const portCfg = electronCfg?.GOOSE_PORT ?? window.appConfig?.get('GOSE_PORT'); + let base = null; + if (hostCfg) { + base = String(hostCfg); + if (!base.startsWith('http://') && !base.startsWith('https://')) + base = `http://${base}`; + base = base.replace(/\/+$/g, ''); + } else if (window.location && window.location.origin) { + base = window.location.origin.replace(/\/+$/g, ''); + } else { + base = 'http://127.0.0.1:17123'; + } + + const url = portCfg + ? `${base}:${portCfg}/config/custom-providers/${currentProvider.name}` + : `${base}/config/custom-providers/${currentProvider.name}`; + const res = await fetch(url, { headers: { 'X-Secret-Key': secretKey } }); + if (res.ok) { + const body = await res.json(); + setConfigValues((prev) => ({ + ...prev, + display_name: body.display_name ?? prev.display_name, + description: body.description ?? prev.description, + headers: body.headers ? JSON.stringify(body.headers) : prev.headers, + timeout_seconds: body.timeout_seconds + ? String(body.timeout_seconds) + : prev.timeout_seconds, + supports_streaming: + body.supports_streaming !== undefined + ? body.supports_streaming + : prev.supports_streaming, + })); + } + } catch (err) { + console.debug('Failed to fetch custom provider JSON', err); + } + } catch (err) { + console.debug(err); + } + })(); + } } }, [isOpen, currentProvider]); @@ -85,6 +177,9 @@ export default function ProviderConfigurationModal() { setValidationErrors({}); // Validation logic + + // Response body placeholder used for toast details + let responseBody: string = ''; const parameters = currentProvider.metadata.config_keys || []; const errors: Record = {}; @@ -107,13 +202,295 @@ export default function ProviderConfigurationModal() { } try { - // Wait for the submission to complete - await SubmitHandler(upsert, currentProvider, configValues); + // If this is a custom provider, call the server update endpoint which + // writes provider settings to the JSON file (not the global config.yaml). + const isCustomProvider = currentProvider.name.startsWith('custom_'); + if (isCustomProvider) { + // Build update payload by mapping known parameter names + type UpdatePayload = { + api_key?: string; + api_url?: string; + models?: string[]; + supports_streaming?: boolean; + display_name?: string; + description?: string; + headers?: Record | null; + timeout_seconds?: number | null; + }; + const payload: UpdatePayload = {}; + for (const param of currentProvider.metadata.config_keys || []) { + const value = configValues[param.name]; + if (value === undefined) continue; + const lower = param.name.toLowerCase(); + if (lower.includes('api_key')) { + // Don't send sentinel values that indicate "secret present" — only include + // the API key when the user explicitly provided/changed it. + if (value !== SECRET_PRESENT_SENTINEL) { + payload.api_key = String(value); + } + } else if ( + lower.includes('api_url') || + lower.includes('host') || + lower.includes('base_url') + ) { + payload.api_url = String(value); + } else if (lower.includes('models')) { + // accept comma-separated models or array + if (Array.isArray(value)) { + payload.models = value; + } else { + payload.models = String(value) + .split(',') + .map((m) => m.trim()) + .filter(Boolean); + } + } else if (param.name.toLowerCase().includes('supports_streaming')) { + payload.supports_streaming = String(value).toLowerCase() === 'true'; + } + } - // Close the modal before triggering refreshes to avoid UI issues - closeModal(); + // allow display_name override from a form field + if (configValues['display_name']) { + payload.display_name = String(configValues['display_name']); + } + + // include editable JSON fields when present + if (configValues['description']) { + payload.description = String(configValues['description']); + } + if (configValues['headers']) { + try { + const parsed = JSON.parse(String(configValues['headers'])); + if (typeof parsed === 'object' && parsed !== null) + payload.headers = parsed as Record; + } catch (err: unknown) { + // ignore invalid JSON; server will validate + console.warn('Invalid headers JSON, skipping headers update', err); + } + } + if (configValues['timeout_seconds']) { + const n = Number(configValues['timeout_seconds']); + if (!Number.isNaN(n)) payload.timeout_seconds = n; + } + + // Allow explicit supports_streaming toggle from the form even if it's not listed + // in the provider's metadata.config_keys (custom provider JSON field). + if (configValues['supports_streaming'] !== undefined) { + // normalize boolean/string values to a boolean + payload.supports_streaming = + String(configValues['supports_streaming']).toLowerCase() === 'true'; + } - // Call onSubmit callback if provided (from modal props) + // Fallback: try to detect any explicit per-provider base url key like CUSTOM__BASE_URL + if (!payload.api_url) { + for (const k of Object.keys(configValues)) { + if (k.toUpperCase().endsWith('_BASE_URL') || k.toLowerCase().includes('base_url')) { + payload.api_url = String(configValues[k]); + break; + } + } + } + + // If payload is still empty, warn and return + if ( + !payload.api_url && + !payload.api_key && + !payload.models && + !payload.display_name && + payload.supports_streaming === undefined + ) { + console.warn( + '[ProviderConfiguationModal] nothing to update for custom provider', + currentProvider.name + ); + // Keep modal open to let user edit fields + return; + } + + try { + const secretKey = await window.electron.getSecretKey(); + + // Determine server base URL. Prefer electron main-provided config, then appConfig, + // then the page origin if it is http(s), otherwise fallback to localhost default. + const electronCfg = + window.electron && window.electron.getConfig ? window.electron.getConfig() : null; + const hostCfg = electronCfg?.GOOSE_API_HOST ?? window.appConfig?.get('GOSE_API_HOST'); + const portCfg = electronCfg?.GOOSE_PORT ?? window.appConfig?.get('GOSE_PORT'); + + let base: string | null = null; + if (hostCfg) { + base = String(hostCfg); + if (!base.startsWith('http://') && !base.startsWith('https://')) { + base = `http://${base}`; + } + base = base.replace(/\/+$/g, ''); + } else if ( + window.location && + window.location.origin && + (window.location.origin.startsWith('http://') || + window.location.origin.startsWith('https://')) + ) { + base = window.location.origin.replace(/\/+$/g, ''); + } else { + // Last resort fallback (best-effort). This will often be correct in dev. + base = 'http://127.0.0.1:17123'; + } + + // Prefer using the generated API client if available + let url = portCfg + ? `${base}:${portCfg}/config/custom-providers/${currentProvider.name}` + : `${base}/config/custom-providers/${currentProvider.name}`; + console.info('[ProviderConfiguationModal] PUT', url, payload); + + // Response body placeholder (declared in outer scope so it can be used below) + let responseBody = ''; + + // Execute update request and capture response body for richer feedback + let res: Response; + try { + // Attempt to use generated client if present (typed functions generated into ui/api) + // If not available, fall back to fetch. + const sdk = await import('../../../../api'); + if ( + sdk && + (sdk as unknown as { createCustomProvider?: unknown }).createCustomProvider + ) { + // If the SDK exposes a put for this path, it will be via generated functions; fallback to fetch + res = await fetch(url, { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + 'X-Secret-Key': secretKey, + }, + body: JSON.stringify(payload), + }); + } else { + res = await fetch(url, { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + 'X-Secret-Key': secretKey, + }, + body: JSON.stringify(payload), + }); + } + } catch (e: unknown) { + console.debug(e); + // fallback to direct fetch + res = await fetch(url, { + method: 'PUT', + headers: { + 'Content-Type': 'application/json', + 'X-Secret-Key': secretKey, + }, + body: JSON.stringify(payload), + }); + } + + // Read the response text (may be empty or JSON) + try { + responseBody = await res.text(); + } catch { + // ignore + responseBody = ''; + } + + if (!res.ok) { + throw new Error(`Update failed: ${res.status} ${responseBody}`); + } + } catch (err: unknown) { + console.error('Failed to update custom provider:', err); + // Keep modal open + return; + } + + // Show a success toast with expandable details (include server response) and refresh providers + try { + let parsedResponse: unknown = responseBody; + try { + parsedResponse = responseBody ? JSON.parse(responseBody) : ''; + } catch (parseErr) { + // ignore parse errors + void parseErr; + } + + const detailsObj = { + request: payload, + response: parsedResponse, + }; + + const details = JSON.stringify(detailsObj, null, 2); + + // Create a toast and ensure it does not auto-close while the details are expanded. + // We capture the returned toast id and pause/resume the auto-close timer when the
+ // element is toggled. Also disable closeOnClick so clicking the summary doesn't dismiss it. + let toastId: number | string | undefined; + + type DetailsToggleEvent = React.SyntheticEvent & { currentTarget: { open?: boolean } }; + + const onDetailsToggle = (e: DetailsToggleEvent) => { + const isOpen = !!e.currentTarget?.open; + if (!toastId) return; + try { + if (isOpen) { + // Pause the toast auto-close while details are open + // Best-effort pause (may not exist on this runtime or in types) + (toast as unknown as { pause?: (id?: string | number) => void }).pause?.(toastId); + } else { + // Resume when collapsed + // Best-effort resume (may not exist on this runtime or in types) + (toast as unknown as { resume?: (id?: string | number) => void }).resume?.(toastId); + } + } catch (err: unknown) { + // best-effort; ignore if pause/resume unavailable + void err; + } + }; + + const content = ( +
+ Custom provider updated +
Changes were saved to the provider JSON.
+
+ Show details + 
{details}
+
+
+ ); + + // Prevent clicking the details from closing the toast and give it a short autoClose by default + toastId = toast.success(content, { closeOnClick: false, autoClose: 5000 }); + } catch (err: unknown) { + console.warn('Failed to render toast with details:', err); + // Fallback: simple console log + console.info('Custom provider updated:', payload, 'response:', responseBody); + } + + // Prefer caller-provided refresh callback, but also refresh global provider list as a fallback + if (modalProps.onSubmit) { + try { + modalProps.onSubmit(configValues as FormValues); + } catch (e: unknown) { + console.debug(e); + console.warn('modalProps.onSubmit threw an error:', e); + } + } + + try { + // Force the shared provider list to refresh so any consumers update + await getProviders(true); + } catch (e: unknown) { + console.debug(e); + console.warn('Failed to refresh global providers after update:', e); + } + + closeModal(); + return; + } + + // Fallback for built-in providers: use existing submit handler + await SubmitHandler(upsert, currentProvider, configValues); + closeModal(); if (modalProps.onSubmit) { modalProps.onSubmit(configValues as FormValues); } diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx index 9ce694fa958b..faef00a19c04 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/CustomProviderForm.tsx @@ -21,7 +21,7 @@ export default function CustomProviderForm({ onSubmit, onCancel }: CustomProvide const [providerType, setProviderType] = useState('openai_compatible'); const [displayName, setDisplayName] = useState(''); const [apiUrl, setApiUrl] = useState(''); - const [apiKey, setApiKey] = useState(''); + const [apiKey, setApiKey] = useState(''); // for new creation flow (no sentinel expected) const [models, setModels] = useState(''); const [isLocalModel, setIsLocalModel] = useState(false); const [supportsStreaming, setSupportsStreaming] = useState(true); diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx index 6f8e2dd69476..7a857124f8ae 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/forms/DefaultProviderSetupForm.tsx @@ -1,5 +1,9 @@ import React, { useEffect, useMemo, useState, useCallback } from 'react'; + +import { SECRET_PRESENT_SENTINEL } from '../../../../../../utils/secretConstants'; + import { Input } from '../../../../../ui/input'; +import { Checkbox } from '@radix-ui/themes'; import { useConfig } from '../../../../../ConfigContext'; // Adjust this import path as needed import { ProviderDetails, ConfigKey } from '../../../../../../api'; @@ -10,6 +14,8 @@ interface DefaultProviderSetupFormProps { setConfigValues: React.Dispatch>>; provider: ProviderDetails; validationErrors: ValidationErrors; + // Optional callback invoked when a field is edited so parent can clear errors + onFieldChange?: (name: string, value: string) => void; } export default function DefaultProviderSetupForm({ @@ -17,6 +23,7 @@ export default function DefaultProviderSetupForm({ setConfigValues, provider, validationErrors = {}, + onFieldChange, }: DefaultProviderSetupFormProps) { const parameters = useMemo( () => provider.metadata.config_keys || [], @@ -25,65 +32,64 @@ export default function DefaultProviderSetupForm({ const [isLoading, setIsLoading] = useState(true); const { read } = useConfig(); - console.log('configValues default form', configValues); - // Initialize values when the component mounts or provider changes const loadConfigValues = useCallback(async () => { + // If there are no parameters, nothing to load + if (parameters.length === 0) { + setIsLoading(false); + return; + } + setIsLoading(true); - const newValues = { ...configValues }; - // Try to load actual values from config for each parameter that is not secret + // Collect responses per parameter without relying on current configValues + const responses: Record = {}; + for (const parameter of parameters) { try { - // Check if there's a stored value in the config system const configKey = `${parameter.name}`; const configResponse = await read(configKey, parameter.secret || false); if (configResponse) { - newValues[parameter.name] = parameter.secret ? 'true' : String(configResponse); - } else if ( - parameter.default !== undefined && - parameter.default !== null && - !configValues[parameter.name] - ) { - // Fall back to default value if no config value exists - newValues[parameter.name] = String(parameter.default); + responses[parameter.name] = parameter.secret + ? SECRET_PRESENT_SENTINEL + : String(configResponse); + } else if (parameter.default !== undefined && parameter.default !== null) { + responses[parameter.name] = String(parameter.default); } } catch (error) { console.error(`Failed to load config for ${parameter.name}:`, error); - // Fall back to default if read operation fails - if ( - parameter.default !== undefined && - parameter.default !== null && - !configValues[parameter.name] - ) { - newValues[parameter.name] = String(parameter.default); + if (parameter.default !== undefined && parameter.default !== null) { + responses[parameter.name] = String(parameter.default); } } } - // Update state with loaded values - setConfigValues((prev) => ({ - ...prev, - ...newValues, - })); + // Merge responses into state but do not overwrite user-entered values + setConfigValues((prev) => { + const merged = { ...prev }; + for (const k of Object.keys(responses)) { + if (merged[k] === undefined || merged[k] === null || merged[k] === '') { + merged[k] = responses[k]; + } + } + return merged; + }); + setIsLoading(false); - }, [configValues, parameters, read, setConfigValues]); + }, [parameters, read, setConfigValues]); + // Load configuration once on mount (modal open). Parent will remount the component + // when provider changes so a mount-time load is sufficient and avoids loops. useEffect(() => { loadConfigValues(); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - // Filter parameters to only show required ones - const requiredParameters = useMemo(() => { - return parameters.filter((param) => param.required === true); - }, [parameters]); + // Show all parameters (required and optional) + const visibleParameters = useMemo(() => parameters, [parameters]); - // TODO: show all params, not just required ones - // const allParameters = useMemo(() => { - // return parameters; - // }, [parameters]); + const currentProviderIsCustom = provider.name && provider.name.startsWith('custom_'); // Helper function to generate appropriate placeholder text const getPlaceholder = (parameter: ConfigKey): string => { @@ -118,19 +124,30 @@ export default function DefaultProviderSetupForm({ .trim(); }; - if (isLoading) { - return
Loading configuration values...
; - } + const handleChange = (parameter: ConfigKey, value: string) => { + setConfigValues((prev) => ({ + ...prev, + [parameter.name]: value, + })); + + // Let parent clear any validation errors for this field and any submission error + if (onFieldChange) onFieldChange(parameter.name, value); + }; - console.log('required params', requiredParameters); return (
- {requiredParameters.length === 0 ? ( + {isLoading && ( +
+ Loading configuration values... +
+ )} + + {visibleParameters.length === 0 ? (
- No required configuration for this provider. + No configuration required for this provider.
) : ( - requiredParameters.map((parameter) => ( + visibleParameters.map((parameter) => (
)) )} + {/* Additional editable custom-provider fields (description, headers, timeout) */} + {currentProviderIsCustom && ( + <> +
+ + + setConfigValues((prev) => ({ ...prev, description: e.target.value })) + } + placeholder="Optional description" + className="w-full h-14 px-4" + /> +
+ +
+ + setConfigValues((prev) => ({ ...prev, headers: e.target.value }))} + placeholder='{"Authorization":"Bearer ..."}' + className="w-full h-14 px-4" + /> +
+ +
+ + + setConfigValues((prev) => ({ ...prev, timeout_seconds: e.target.value })) + } + placeholder="30" + className="w-full h-14 px-4" + /> +
+ +
+ + setConfigValues((prev) => ({ ...prev, supports_streaming: String(checked) })) + } + /> + +
+ + )}
); } diff --git a/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx b/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx index 3356f4a8af88..300ba8aa1b7e 100644 --- a/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx +++ b/ui/desktop/src/components/settings/providers/modal/subcomponents/handlers/DefaultSubmitHandler.tsx @@ -1,3 +1,5 @@ +import { SECRET_PRESENT_SENTINEL } from '../../../../../../utils/secretConstants'; + /** * Standalone function to submit provider configuration * Useful for components that don't want to use the hook @@ -74,6 +76,12 @@ export const DefaultSubmitHandler = async ( // Explicitly define is_secret as a boolean (true/false) const isSecret = parameter.secret === true; + // If this is a secret value and the sentinel indicates the secret is present + // but unchanged, skip writing to avoid overwriting the keyring with the sentinel. + if (isSecret && value === SECRET_PRESENT_SENTINEL) { + return Promise.resolve(); + } + // Pass the is_secret flag from the parameter definition return upsertFn(configKey, value, isSecret); } diff --git a/ui/desktop/src/utils/secretConstants.ts b/ui/desktop/src/utils/secretConstants.ts new file mode 100644 index 000000000000..00a9a1a23b66 --- /dev/null +++ b/ui/desktop/src/utils/secretConstants.ts @@ -0,0 +1 @@ +export const SECRET_PRESENT_SENTINEL = '__GOOSE_SECRET_PRESENT__';