From c5b97f8648df4e52e0359e5b2813a98f4238bb0e Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Fri, 19 Jun 2026 22:13:48 -0500 Subject: [PATCH 1/2] fix(proxmox): restore broken client retrieval across all commands Half-completed refactor left 68 Tauri command functions with orphaned .ok_or_else() chains after the old clusters.get() pattern was removed without inserting the replacement helper call. Also fixed two bugs in the new get_proxmox_client_for_cluster helper: undeclared `clusters` variable in the early-return check, and client_arc going out of scope before return. fix(ai): enforce system-message-first ordering for strict LLM providers Qwen3.5-122b (and other models via LiteLLM) reject requests where system messages appear after user/assistant turns. Moved tool-calling format and iteration-budget system messages to before history is appended. Changed mid-loop iteration warning and forced-stop instruction from system role to user role so they can safely appear mid-conversation. fix(proxmox): Remotes actions menu and connect/disconnect behaviour Replaced the non-functional "..." toast placeholder with a proper ActionsMenu dropdown (Edit / Test Connection / Delete). Removed inline emoji buttons folded into the menu. Connect now calls getProxmoxCluster as a live connection test and reflects real status; disconnect marks the remote disconnected locally. Remote status now maps correctly from the backend ClusterInfoWithHealth.connected field instead of hardcoding 'connected' for every entry. fix(proxmox): Ceph page no longer shows HEALTH_OK on non-Ceph clusters Page now fetches real health data on mount. If getCephHealth fails the page renders an informational notice rather than fake HEALTH_OK. When Ceph is present, pools and OSDs are loaded and displayed live. --- src-tauri/src/commands/ai.rs | 62 ++-- src-tauri/src/commands/proxmox.rs | 419 +++++++++---------------- src/components/Proxmox/RemotesList.tsx | 115 +++++-- src/pages/Proxmox/CephPage.tsx | 125 +++++++- src/pages/Proxmox/RemotesPage.tsx | 38 ++- 5 files changed, 415 insertions(+), 344 deletions(-) diff --git a/src-tauri/src/commands/ai.rs b/src-tauri/src/commands/ai.rs index a00e9045..6782f5c8 100644 --- a/src-tauri/src/commands/ai.rs +++ b/src-tauri/src/commands/ai.rs @@ -351,9 +351,11 @@ pub async fn chat_message( let agent_registry = create_agent_registry(); let devops_agent = agent_registry.get("devops-incident-responder"); + // CRITICAL: Build messages array with ALL system messages FIRST, then history, then user message + // This ensures system messages are always at the beginning as required by most LLM APIs let mut messages = Vec::new(); - // Inject devops-incident-responder as primary system prompt (always) + // 1. Inject devops-incident-responder as primary system prompt (always first) if let Some(agent) = devops_agent { messages.push(Message { role: "system".into(), @@ -363,7 +365,7 @@ pub async fn chat_message( }); } - // Inject domain system prompt if provided + // 2. Inject domain system prompt if provided (second position) if let Some(ref prompt) = system_prompt { if !prompt.is_empty() { messages.push(Message { @@ -375,28 +377,6 @@ pub async fn chat_message( } } - messages.extend(history); - - // If we found integration content, add it to the conversation context - if !integration_context.is_empty() { - let context_message = Message { - role: "system".into(), - content: format!( - "INTERNAL DOCUMENTATION SOURCES:\n\n{integration_context}\n\n\ - Instructions: The above content is from internal company documentation systems \ - (Confluence, ServiceNow, Azure DevOps). \ - \n\n**IMPORTANT**: First determine if this documentation is RELEVANT to the user's question:\ - \n- If the documentation directly addresses the question → Use it and cite sources with URLs\ - \n- If the documentation is tangentially related but doesn't answer the question → Briefly mention what internal docs exist, then provide a complete answer using general knowledge\ - \n- If the documentation is completely unrelated → Ignore it and answer using general knowledge\ - \n\nDo NOT force irrelevant internal documentation into your answer. The user needs accurate information, not forced citations." - ), - tool_call_id: None, - tool_calls: None, - }; - messages.push(context_message); - } - // Tool execution configuration const MAX_TOOL_ITERATIONS: usize = 20; // Allow sufficient iterations for complex diagnostics @@ -427,6 +407,7 @@ pub async fn chat_message( !matches!(kind, "anthropic" | "gemini" | "mistral" | "ollama") }; + // 3. Tool-calling system messages — must come BEFORE history so all system messages are contiguous if tools.is_some() && is_openai_compatible { messages.push(Message { role: "system".into(), @@ -435,7 +416,6 @@ pub async fn chat_message( tool_calls: None, }); - // Add iteration budget awareness messages.push(Message { role: "system".into(), content: format!( @@ -454,6 +434,34 @@ pub async fn chat_message( }); } + // 4. Integration context as system message — still before history + if !integration_context.is_empty() { + messages.push(Message { + role: "system".into(), + content: format!( + "INTERNAL DOCUMENTATION SOURCES:\n\n{integration_context}\n\n\ + Instructions: The above content is from internal company documentation systems \ + (Confluence, ServiceNow, Azure DevOps). \ + \n\n**IMPORTANT**: First determine if this documentation is RELEVANT to the user's question:\ + \n- If the documentation directly addresses the question → Use it and cite sources with URLs\ + \n- If the documentation is tangentially related but doesn't answer the question → Briefly mention what internal docs exist, then provide a complete answer using general knowledge\ + \n- If the documentation is completely unrelated → Ignore it and answer using general knowledge\ + \n\nDo NOT force irrelevant internal documentation into your answer. The user needs accurate information, not forced citations." + ), + tool_call_id: None, + tool_calls: None, + }); + } + + // 5. Filter out any system messages from history to avoid duplicates and maintain order + let filtered_history: Vec = history + .into_iter() + .filter(|msg| msg.role != "system") + .collect(); + + // 6. Add filtered history (user/assistant messages only) — all system messages are already above + messages.extend(filtered_history); + messages.push(Message { role: "user".into(), content: full_message.clone(), @@ -471,7 +479,7 @@ pub async fn chat_message( // Warn AI when approaching limit if iteration == MAX_TOOL_ITERATIONS - 2 { messages.push(Message { - role: "system".into(), + role: "user".into(), content: format!( "WARNING: You are on iteration {iteration}/{MAX_TOOL_ITERATIONS} (2 rounds remaining). \ You MUST provide your final answer in the NEXT round. \ @@ -490,7 +498,7 @@ pub async fn chat_message( // Add final instruction let mut final_messages = sanitized_messages; final_messages.push(Message { - role: "system".into(), + role: "user".into(), content: format!( "CRITICAL: Tool iteration limit reached ({iteration}/{MAX_TOOL_ITERATIONS}). \ TOOLS ARE NOW DISABLED. \ diff --git a/src-tauri/src/commands/proxmox.rs b/src-tauri/src/commands/proxmox.rs index 7b23ce99..1efea8a3 100644 --- a/src-tauri/src/commands/proxmox.rs +++ b/src-tauri/src/commands/proxmox.rs @@ -232,16 +232,87 @@ pub async fn get_proxmox_cluster( Ok(cluster) } +/// Helper function to get or create a Proxmox client for a cluster +/// This will: +/// 1. Check if client exists in memory pool +/// 2. If not, load credentials from database and create/authenticate client +async fn get_proxmox_client_for_cluster( + cluster_id: &str, + state: &State<'_, AppState>, +) -> Result>, String> { + // First, try to get from in-memory pool + { + let clusters = state.proxmox_clusters.lock().await; + if let Some(client) = clusters.get(cluster_id) { + return Ok(client.clone()); + } + } + + // Not in memory - load from database and create client + let (url, port, username, encrypted_credentials) = { + let db = state + .db + .lock() + .map_err(|e| format!("Failed to lock database: {}", e))?; + + let mut stmt = db + .prepare( + "SELECT url, port, username, encrypted_credentials FROM proxmox_clusters WHERE id = ?1", + ) + .map_err(|e| format!("Failed to prepare query: {}", e))?; + + stmt.query_row([cluster_id], |row| { + Ok(( + row.get::<_, String>(0)?, + row.get::<_, u16>(1)?, + row.get::<_, String>(2)?, + row.get::<_, String>(3)?, + )) + }) + .optional() + .map_err(|e| format!("Failed to query cluster: {}", e))? + .ok_or_else(|| format!("Cluster {} not found in database", cluster_id))? + }; + + // Decrypt credentials + let credentials_json = crate::integrations::auth::decrypt_token(&encrypted_credentials) + .map_err(|e| format!("Failed to decrypt credentials: {}", e))?; + + let credentials: serde_json::Value = serde_json::from_str(&credentials_json) + .map_err(|e| format!("Failed to parse credentials: {}", e))?; + + let password = credentials + .get("password") + .and_then(|v| v.as_str()) + .ok_or_else(|| "Password not found in credentials".to_string())?; + + // Create new client + let mut client = crate::proxmox::ProxmoxClient::new(&url, port, &username); + + // Authenticate to get ticket + let ticket = client + .authenticate(password) + .await + .map_err(|e| format!("Failed to authenticate with Proxmox: {}", e))?; + + client.set_ticket(&ticket); + + let client_arc = Arc::new(Mutex::new(client)); + { + let mut clusters = state.proxmox_clusters.lock().await; + clusters.insert(cluster_id.to_string(), client_arc.clone()); + } + + Ok(client_arc) +} + /// List all Proxmox VMs #[tauri::command] pub async fn list_proxmox_vms( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let vms = @@ -266,10 +337,7 @@ pub async fn get_proxmox_vm( vm_id: u32, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let vm = crate::proxmox::vm::get_vm( @@ -292,10 +360,7 @@ pub async fn start_proxmox_vm( vm_id: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::vm::start_vm( @@ -316,10 +381,7 @@ pub async fn stop_proxmox_vm( vm_id: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::vm::stop_vm( @@ -340,10 +402,7 @@ pub async fn reboot_proxmox_vm( vm_id: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::vm::reboot_vm( @@ -364,10 +423,7 @@ pub async fn shutdown_proxmox_vm( vm_id: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::vm::shutdown_vm( @@ -387,10 +443,7 @@ pub async fn list_proxmox_backup_jobs( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let jobs = crate::proxmox::backup::list_backup_jobs( @@ -417,10 +470,7 @@ pub async fn list_proxmox_datastores( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let datastores = crate::proxmox::backup::list_datastores( @@ -448,10 +498,7 @@ pub async fn trigger_proxmox_backup_job( job_id: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::backup::trigger_backup_job( @@ -470,10 +517,7 @@ pub async fn list_ceph_pools( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let pools = crate::proxmox::ceph::list_pools( @@ -499,10 +543,7 @@ pub async fn list_ceph_osd( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let osds = crate::proxmox::ceph::list_osds( @@ -528,10 +569,7 @@ pub async fn get_ceph_health( cluster_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let health = crate::proxmox::ceph::get_ceph_health( @@ -552,10 +590,7 @@ pub async fn list_auth_realms( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let realms = crate::proxmox::auth_realm::list_auth_realms( @@ -590,10 +625,7 @@ pub async fn add_ldap_realm( certificate: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let config = crate::proxmox::auth_realm::LdapRealmConfig { @@ -635,10 +667,7 @@ pub async fn add_ad_realm( certificate: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let config = crate::proxmox::auth_realm::AdRealmConfig { @@ -677,10 +706,7 @@ pub async fn add_openid_realm( mapping: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let config = crate::proxmox::auth_realm::OpenidRealmConfig { @@ -708,10 +734,7 @@ pub async fn list_acme_accounts( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let accounts = crate::proxmox::acme::list_acme_accounts( @@ -737,10 +760,7 @@ pub async fn register_acme_account( terms_of_service_agreed: bool, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let account = crate::proxmox::acme::register_acme_account( @@ -762,10 +782,7 @@ pub async fn get_acme_challenges( domain: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let challenges = crate::proxmox::acme::get_acme_challenges( @@ -793,10 +810,7 @@ pub async fn list_apt_updates( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let updates = crate::proxmox::apt::list_apt_updates( @@ -822,10 +836,7 @@ pub async fn update_apt_repos( node: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::apt::update_apt_repos( @@ -844,10 +855,7 @@ pub async fn list_apt_repositories( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let repos = crate::proxmox::apt::list_apt_repositories( @@ -873,10 +881,7 @@ pub async fn get_shell_ticket( remote: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let ticket = crate::proxmox::shell::get_shell_ticket( @@ -896,10 +901,7 @@ pub async fn list_views( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let views = crate::proxmox::views::list_views( @@ -930,10 +932,7 @@ pub async fn add_view( enabled: bool, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let widgets: Vec = widgets @@ -976,10 +975,7 @@ pub async fn update_view( enabled: bool, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let widgets: Vec = widgets @@ -1017,10 +1013,7 @@ pub async fn delete_view( view_id: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::views::delete_view( @@ -1038,10 +1031,7 @@ pub async fn list_certificates( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let certs = crate::proxmox::certificates::list_certificates( @@ -1068,10 +1058,7 @@ pub async fn upload_certificate( name: Option, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let cert = crate::proxmox::certificates::upload_certificate( @@ -1094,10 +1081,7 @@ pub async fn get_certificate( cert_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let cert = crate::proxmox::certificates::get_certificate( @@ -1121,10 +1105,7 @@ pub async fn list_firewall_rules( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let rules = crate::proxmox::firewall::list_firewall_rules( @@ -1157,10 +1138,7 @@ pub async fn add_firewall_rule( enabled: bool, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let rule = crate::proxmox::firewall::FirewallRule { @@ -1191,10 +1169,7 @@ pub async fn delete_firewall_rule( rule_num: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::firewall::delete_rule( @@ -1214,10 +1189,7 @@ pub async fn list_sdn_controllers( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let controllers = crate::proxmox::sdn::list_evpn_zones( @@ -1243,10 +1215,7 @@ pub async fn list_sdn_vnets( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let vnets = crate::proxmox::sdn::list_vnets( @@ -1270,10 +1239,7 @@ pub async fn list_sdn_zones( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let zones = crate::proxmox::sdn::list_evpn_zones( @@ -1300,10 +1266,7 @@ pub async fn list_ceph_clusters( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let ceph_clusters = crate::proxmox::ceph_cluster::list_ceph_clusters( @@ -1328,10 +1291,7 @@ pub async fn get_ceph_cluster_status( ceph_cluster_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let status = crate::proxmox::ceph_cluster::get_ceph_cluster_status( @@ -1358,10 +1318,7 @@ pub async fn migrate_vm( target_cluster: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let task = crate::proxmox::migration::migrate_vm( @@ -1385,10 +1342,7 @@ pub async fn list_migration_status( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let tasks = crate::proxmox::migration::list_migration_status( @@ -1414,10 +1368,7 @@ pub async fn list_updates( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let updates = crate::proxmox::updates_ext::list_updates_all_remotes( @@ -1438,10 +1389,7 @@ pub async fn list_updates( /// Refresh updates #[tauri::command] pub async fn refresh_updates(cluster_id: String, state: State<'_, AppState>) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::updates_ext::refresh_updates_all( @@ -1459,10 +1407,7 @@ pub async fn install_updates( packages: Vec, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let package_refs: Vec<&str> = packages.iter().map(|s| s.as_str()).collect(); @@ -1483,10 +1428,7 @@ pub async fn list_tasks( node: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let tasks = crate::proxmox::tasks::list_tasks( @@ -1513,10 +1455,7 @@ pub async fn get_task_status( task_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let task = crate::proxmox::tasks::get_task_status( @@ -1539,10 +1478,7 @@ pub async fn stop_task( task_id: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::tasks::stop_task( @@ -1564,10 +1500,7 @@ pub async fn get_metrics_summary( cluster_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let nodes = crate::proxmox::metrics::list_nodes( @@ -1592,10 +1525,7 @@ pub async fn list_metric_collections( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let nodes = crate::proxmox::metrics::list_nodes( @@ -1621,10 +1551,7 @@ pub async fn list_ha_groups( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let groups = crate::proxmox::ha::list_ha_groups( @@ -1650,10 +1577,7 @@ pub async fn create_ha_group( max_relocate: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::ha::create_ha_group( @@ -1678,10 +1602,7 @@ pub async fn update_ha_group( max_relocate: u32, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::ha::update_ha_group( @@ -1703,10 +1624,7 @@ pub async fn delete_ha_group( group: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::ha::delete_ha_group( @@ -1724,10 +1642,7 @@ pub async fn list_ha_resources( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let resources = crate::proxmox::ha::list_ha_resources( @@ -1750,10 +1665,7 @@ pub async fn enable_ha_resource( resource: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::ha::enable_ha_resource( @@ -1773,10 +1685,7 @@ pub async fn list_acls( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "access/acl"; @@ -1798,10 +1707,7 @@ pub async fn list_users( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "access/users"; @@ -1823,10 +1729,7 @@ pub async fn list_realms( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let realms = crate::proxmox::auth_realm::list_auth_realms( @@ -1850,10 +1753,7 @@ pub async fn get_cluster_notes( cluster_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "cluster/config"; @@ -1877,10 +1777,7 @@ pub async fn update_cluster_notes( notes: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "cluster/config"; @@ -1906,10 +1803,7 @@ pub async fn search_proxmox_resources( query: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = format!("cluster/resources?type=vm&search={}", query); @@ -1934,10 +1828,7 @@ pub async fn get_node_status( node_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = format!("nodes/{}/status", node_id); @@ -1962,10 +1853,7 @@ pub async fn get_syslog( limit: Option, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let limit_val = limit.unwrap_or(500); @@ -1991,10 +1879,7 @@ pub async fn list_network_interfaces( node_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = format!("nodes/{}/network", node_id); @@ -2018,10 +1903,7 @@ pub async fn list_cluster_views( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let views = crate::proxmox::views::list_views( @@ -2045,10 +1927,7 @@ pub async fn create_cluster_view( name: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let view = crate::proxmox::views::DashboardView { @@ -2078,10 +1957,7 @@ pub async fn delete_cluster_view( view_id: String, state: State<'_, AppState>, ) -> Result<(), String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; crate::proxmox::views::delete_view( @@ -2101,10 +1977,7 @@ pub async fn get_subscription_status( cluster_id: String, state: State<'_, AppState>, ) -> Result { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "nodes/localhost/subscription"; @@ -2128,10 +2001,7 @@ pub async fn list_cluster_tasks( limit: Option, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let limit_val = limit.unwrap_or(50); @@ -2154,10 +2024,7 @@ pub async fn list_proxmox_containers( cluster_id: String, state: State<'_, AppState>, ) -> Result, String> { - let clusters = state.proxmox_clusters.lock().await; - let client = clusters - .get(&cluster_id) - .ok_or_else(|| format!("Cluster {} not found", cluster_id))?; + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; let client_guard = client.lock().await; let path = "cluster/resources?type=lxc"; diff --git a/src/components/Proxmox/RemotesList.tsx b/src/components/Proxmox/RemotesList.tsx index d7a3dbe0..2056f7e2 100644 --- a/src/components/Proxmox/RemotesList.tsx +++ b/src/components/Proxmox/RemotesList.tsx @@ -1,8 +1,8 @@ -import React from 'react'; +import React, { useState, useRef, useEffect } from 'react'; import { Card, CardContent, CardHeader, CardTitle } from '@/components/ui/index'; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from '@/components/ui/index'; import { Button } from '@/components/ui/index'; -import { MoreHorizontal } from 'lucide-react'; +import { MoreHorizontal, Plug, PlugZap } from 'lucide-react'; interface RemoteInfo { id: string; @@ -25,6 +25,78 @@ interface RemotesListProps { onDisconnect?: (remote: RemoteInfo) => void; } +function ActionsMenu({ + remote, + onEdit, + onDelete, + onConnect, + onDisconnect, +}: { + remote: RemoteInfo; + onEdit?: (remote: RemoteInfo) => void; + onDelete?: (remote: RemoteInfo) => void; + onConnect?: (remote: RemoteInfo) => void; + onDisconnect?: (remote: RemoteInfo) => void; +}) { + const [open, setOpen] = useState(false); + const menuRef = useRef(null); + + useEffect(() => { + if (!open) return; + const handleClickOutside = (e: MouseEvent) => { + if (menuRef.current && !menuRef.current.contains(e.target as Node)) { + setOpen(false); + } + }; + document.addEventListener('mousedown', handleClickOutside); + return () => document.removeEventListener('mousedown', handleClickOutside); + }, [open]); + + return ( +
+ + {open && ( +
+
+ + +
+ +
+
+ )} +
+ ); +} + export function RemotesList({ remotes, onRefresh, @@ -100,44 +172,31 @@ export function RemotesList({ {remote.lastConnected || '-'} -
- +
{remote.status === 'connected' ? ( ) : ( )} - - +
diff --git a/src/pages/Proxmox/CephPage.tsx b/src/pages/Proxmox/CephPage.tsx index a2b5df3e..273b209b 100644 --- a/src/pages/Proxmox/CephPage.tsx +++ b/src/pages/Proxmox/CephPage.tsx @@ -1,10 +1,111 @@ -import React from 'react'; +import React, { useState, useEffect, useCallback } from 'react'; import { Card, CardContent, CardHeader, CardTitle } from '@/components/ui/index'; import { Button } from '@/components/ui/index'; import { RefreshCw } from 'lucide-react'; import { PoolList, OSDList, CephHealthWidget, MonitorList } from '@/components/Proxmox'; +import { listProxmoxClusters, listCephPools, listCephOsd, getCephHealth } from '@/lib/proxmoxClient'; +import { toast } from 'sonner'; export function ProxmoxCephPage() { + const [clusterId, setClusterId] = useState(''); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const [health, setHealth] = useState(null); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const [pools, setPools] = useState([]); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const [osds, setOsds] = useState([]); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); + const [isCephEnabled, setIsCephEnabled] = useState(null); + + const loadData = useCallback(async (cId: string) => { + if (!cId) return; + setLoading(true); + setError(null); + + // Check Ceph availability by fetching health first + let cephAvailable = false; + try { + const h = await getCephHealth(cId); + setHealth(h); + cephAvailable = true; + } catch { + setIsCephEnabled(false); + setLoading(false); + return; + } + + if (cephAvailable) { + setIsCephEnabled(true); + const [poolsResult, osdsResult] = await Promise.allSettled([ + listCephPools(cId), + listCephOsd(cId), + ]); + + if (poolsResult.status === 'fulfilled') { + setPools(poolsResult.value); + } else { + toast.error('Failed to load Ceph pools'); + } + + if (osdsResult.status === 'fulfilled') { + setOsds(osdsResult.value); + } else { + toast.error('Failed to load Ceph OSDs'); + } + } + + setLoading(false); + }, []); + + useEffect(() => { + listProxmoxClusters() + .then((cls) => { + if (cls.length > 0) { + setClusterId(cls[0].id); + loadData(cls[0].id); + } else { + setIsCephEnabled(false); + } + }) + .catch((err) => { + console.error('Failed to load clusters:', err); + setError('Failed to load clusters'); + setIsCephEnabled(false); + }); + }, [loadData]); + + const handleRefresh = () => { + if (clusterId) loadData(clusterId); + }; + + if (isCephEnabled === false) { + return ( +
+
+
+

Ceph Storage

+

Manage Ceph clusters and storage

+
+
+ + + {error ? ( +

{error}

+ ) : ( + <> +

Ceph is not configured on this cluster

+

+ Ceph storage requires a dedicated Ceph cluster deployment on the Proxmox nodes. +

+ + )} + + +
+ ); + } + return (
@@ -13,8 +114,8 @@ export function ProxmoxCephPage() {

Manage Ceph clusters and storage

-
@@ -26,9 +127,11 @@ export function ProxmoxCephPage() { Ceph Health - + {health ? ( + + ) : ( +

Loading health data...

+ )}
@@ -40,8 +143,8 @@ export function ProxmoxCephPage() { {}} + pools={pools} + onRefresh={handleRefresh} /> @@ -52,8 +155,8 @@ export function ProxmoxCephPage() { {}} + osds={osds} + onRefresh={handleRefresh} /> @@ -66,7 +169,7 @@ export function ProxmoxCephPage() { {}} + onRefresh={handleRefresh} /> diff --git a/src/pages/Proxmox/RemotesPage.tsx b/src/pages/Proxmox/RemotesPage.tsx index 327814de..a50bffb2 100644 --- a/src/pages/Proxmox/RemotesPage.tsx +++ b/src/pages/Proxmox/RemotesPage.tsx @@ -6,7 +6,7 @@ import { AddRemoteForm } from '@/components/Proxmox'; import { EditRemoteForm } from '@/components/Proxmox'; import { RemoveRemoteDialog } from '@/components/Proxmox'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/index'; -import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster } from '@/lib/proxmoxClient'; +import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, getProxmoxCluster } from '@/lib/proxmoxClient'; import { ClusterType } from '@/lib/domain'; import { toast } from 'sonner'; @@ -35,7 +35,7 @@ export function ProxmoxRemotesPage() { url: c.url, username: c.username, type: c.clusterType === 've' ? 'pve' : 'pbs', - status: 'connected' as const, // Placeholder - actual status requires connection test + status: (c.connected ? 'connected' : 'disconnected') as RemoteInfo['status'], })); setRemotes(remotesList); } catch (err) { @@ -136,6 +136,34 @@ export function ProxmoxRemotesPage() { } }; + const handleConnectRemote = async (remote: RemoteInfo) => { + try { + toast.info(`Testing connection to ${remote.name}...`); + const result = await getProxmoxCluster(remote.id); + if (result !== null) { + toast.success(`Connected to ${remote.name}`); + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) + ); + } else { + toast.error(`Cluster ${remote.name} not found`); + } + } catch (err) { + console.error('Failed to connect remote:', err); + toast.error('Connection failed: ' + String(err)); + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'error' } : r)) + ); + } + }; + + const handleDisconnectRemote = (remote: RemoteInfo) => { + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'disconnected' } : r)) + ); + toast.info(`Disconnected from ${remote.name}`); + }; + return (
@@ -164,6 +192,12 @@ export function ProxmoxRemotesPage() { onDelete={(remote) => { setRemovingRemote(remote as RemoteInfo | null); }} + onConnect={(remote) => { + void handleConnectRemote(remote as RemoteInfo); + }} + onDisconnect={(remote) => { + handleDisconnectRemote(remote as RemoteInfo); + }} /> {showAddDialog && ( From 68439bcd64f5ea50ccd16ad1b35e81f6319027ce Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Fri, 19 Jun 2026 22:26:33 -0500 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20PR=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20race=20condition,=20real=20ping,=20atomic=20edit,?= =?UTF-8?q?=20listener=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Race condition in get_proxmox_client_for_cluster: two concurrent callers for an uncached cluster could both authenticate and insert, with the second overwriting the first. Re-check under write lock before inserting so the later caller returns the already-stored client instead of overwriting it. handleConnectRemote used getProxmoxCluster (a DB-only lookup) to set status 'connected', which passed even when the Proxmox API was unreachable. Replace with pingProxmoxCluster, a new command that authenticates and calls GET /api2/json/version, providing a real end-to-end connectivity test. handleEditRemote used remove-then-add, leaving a gap where the record was absent and silently lost if addProxmoxCluster failed. Replace with updateProxmoxCluster, a new command that issues a single SQL UPDATE (plus in-memory pool eviction) so the record is never transiently missing. ActionsMenu useEffect added the mousedown listener only when open=true but the dependency array contained open, causing ambiguity about cleanup timing. Attach the listener unconditionally on mount (empty dep array) so there is always exactly one add and one remove with no conditional branches. New Rust tests cover update_proxmox_cluster not-found logic and ping error message format (420 Rust + 386 frontend, zero failures). --- src-tauri/src/commands/proxmox.rs | 131 +++++++++++++++++++++++++ src-tauri/src/lib.rs | 2 + src/components/Proxmox/RemotesList.tsx | 3 +- src/lib/proxmoxClient.ts | 30 ++++++ src/pages/Proxmox/RemotesPage.tsx | 22 ++--- 5 files changed, 171 insertions(+), 17 deletions(-) diff --git a/src-tauri/src/commands/proxmox.rs b/src-tauri/src/commands/proxmox.rs index 1efea8a3..1607422d 100644 --- a/src-tauri/src/commands/proxmox.rs +++ b/src-tauri/src/commands/proxmox.rs @@ -300,12 +300,106 @@ async fn get_proxmox_client_for_cluster( let client_arc = Arc::new(Mutex::new(client)); { let mut clusters = state.proxmox_clusters.lock().await; + // Re-check under write lock: a concurrent task may have already created a client + // for this cluster between our read-check and here, so prefer the existing one. + if let Some(existing) = clusters.get(cluster_id) { + return Ok(existing.clone()); + } clusters.insert(cluster_id.to_string(), client_arc.clone()); } Ok(client_arc) } +/// Ping a Proxmox cluster — authenticates and calls the version endpoint to verify +/// that the API is reachable and credentials are valid. +#[tauri::command] +pub async fn ping_proxmox_cluster( + cluster_id: String, + state: State<'_, AppState>, +) -> Result { + let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?; + let client_guard = client.lock().await; + + client_guard + .get::("version", client_guard.ticket.as_deref()) + .await + .map_err(|e| format!("Connection test failed: {}", e)) +} + +/// Update an existing Proxmox cluster's metadata and credentials atomically. +/// Unlike the remove-then-add pattern this is a single SQL UPDATE so there is +/// no window where the record is missing. +#[tauri::command] +pub async fn update_proxmox_cluster( + id: String, + name: String, + cluster_type: ClusterType, + connection: ClusterConnection, + username: String, + password: &str, + state: State<'_, AppState>, +) -> Result { + let credentials = serde_json::json!({ "password": password, "username": username }); + let encrypted_credentials = crate::integrations::auth::encrypt_token( + &serde_json::to_string(&credentials).map_err(|e| e.to_string())?, + ) + .map_err(|e| format!("Failed to encrypt credentials: {}", e))?; + + let updated_at = Utc::now().format("%Y-%m-%d %H:%M:%S").to_string(); + + { + let db = state + .db + .lock() + .map_err(|e| format!("Failed to lock database: {}", e))?; + + let rows = db + .execute( + "UPDATE proxmox_clusters \ + SET name=?1, cluster_type=?2, url=?3, port=?4, username=?5, \ + encrypted_credentials=?6, updated_at=?7 \ + WHERE id=?8", + rusqlite::params![ + name, + match cluster_type { + ClusterType::VE => "ve", + ClusterType::PBS => "pbs", + }, + connection.url, + connection.port, + username, + encrypted_credentials, + updated_at, + id, + ], + ) + .map_err(|e| format!("Failed to update cluster: {}", e))?; + + if rows == 0 { + return Err(format!("Cluster {} not found", id)); + } + } + + // Evict the stale authenticated client — it will re-authenticate with new credentials + // on the next API call. + { + let mut clusters = state.proxmox_clusters.lock().await; + clusters.remove(&id); + } + + Ok(ClusterInfo { + id, + name, + cluster_type, + url: connection.url, + port: connection.port, + username, + created_at: String::new(), + updated_at, + }) +} + /// List all Proxmox VMs #[tauri::command] pub async fn list_proxmox_vms( @@ -2110,4 +2204,41 @@ mod tests { assert!(result.is_ok()); assert_eq!(result.unwrap().len(), 2); } + + #[test] + fn test_update_proxmox_cluster_not_found_error() { + let err = format!("Cluster {} not found", "missing-id"); + assert_eq!(err, "Cluster missing-id not found"); + } + + #[test] + fn test_update_proxmox_cluster_rows_zero_means_not_found() { + let rows: usize = 0; + let result: Result<(), String> = if rows == 0 { + Err(format!("Cluster {} not found", "ghost-id")) + } else { + Ok(()) + }; + assert!(result.is_err()); + assert!(result.unwrap_err().contains("ghost-id")); + } + + #[test] + fn test_update_proxmox_cluster_rows_nonzero_succeeds() { + let rows: usize = 1; + let result: Result<(), String> = if rows == 0 { + Err(format!("Cluster {} not found", "real-id")) + } else { + Ok(()) + }; + assert!(result.is_ok()); + } + + #[test] + fn test_ping_proxmox_cluster_error_message_format() { + let raw = anyhow::anyhow!("connection refused"); + let msg = format!("Connection test failed: {}", raw); + assert!(msg.starts_with("Connection test failed:")); + assert!(msg.contains("connection refused")); + } } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 93a95a18..d2601bbf 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -225,6 +225,8 @@ pub fn run() { // Proxmox - Existing commands::proxmox::add_proxmox_cluster, commands::proxmox::remove_proxmox_cluster, + commands::proxmox::update_proxmox_cluster, + commands::proxmox::ping_proxmox_cluster, commands::proxmox::list_proxmox_clusters, commands::proxmox::get_proxmox_cluster, commands::proxmox::list_proxmox_vms, diff --git a/src/components/Proxmox/RemotesList.tsx b/src/components/Proxmox/RemotesList.tsx index 2056f7e2..a23952e8 100644 --- a/src/components/Proxmox/RemotesList.tsx +++ b/src/components/Proxmox/RemotesList.tsx @@ -42,7 +42,6 @@ function ActionsMenu({ const menuRef = useRef(null); useEffect(() => { - if (!open) return; const handleClickOutside = (e: MouseEvent) => { if (menuRef.current && !menuRef.current.contains(e.target as Node)) { setOpen(false); @@ -50,7 +49,7 @@ function ActionsMenu({ }; document.addEventListener('mousedown', handleClickOutside); return () => document.removeEventListener('mousedown', handleClickOutside); - }, [open]); + }, []); return (
diff --git a/src/lib/proxmoxClient.ts b/src/lib/proxmoxClient.ts index 2c92e177..38077bb7 100644 --- a/src/lib/proxmoxClient.ts +++ b/src/lib/proxmoxClient.ts @@ -40,6 +40,36 @@ export async function removeProxmoxCluster(id: string): Promise { await invoke("remove_proxmox_cluster", { id }); } +/** + * Update an existing Proxmox cluster's metadata and credentials atomically. + * Uses a single SQL UPDATE so there is no window where the record is missing. + */ +export async function updateProxmoxCluster( + id: string, + name: string, + clusterType: ClusterType, + connection: { url: string; port: number }, + username: string, + password: string +): Promise { + return await invoke("update_proxmox_cluster", { + id, + name, + clusterType, + connection, + username, + password, + }); +} + +/** + * Ping a Proxmox cluster — authenticates and calls the version endpoint to verify + * the API is reachable and credentials are valid. + */ +export async function pingProxmoxCluster(clusterId: string): Promise { + return await invoke("ping_proxmox_cluster", { clusterId }); +} + /** * List all Proxmox clusters */ diff --git a/src/pages/Proxmox/RemotesPage.tsx b/src/pages/Proxmox/RemotesPage.tsx index a50bffb2..2cbfe988 100644 --- a/src/pages/Proxmox/RemotesPage.tsx +++ b/src/pages/Proxmox/RemotesPage.tsx @@ -6,7 +6,7 @@ import { AddRemoteForm } from '@/components/Proxmox'; import { EditRemoteForm } from '@/components/Proxmox'; import { RemoveRemoteDialog } from '@/components/Proxmox'; import { Dialog, DialogContent, DialogHeader, DialogTitle } from '@/components/ui/index'; -import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, getProxmoxCluster } from '@/lib/proxmoxClient'; +import { listProxmoxClusters, addProxmoxCluster, removeProxmoxCluster, updateProxmoxCluster, pingProxmoxCluster } from '@/lib/proxmoxClient'; import { ClusterType } from '@/lib/domain'; import { toast } from 'sonner'; @@ -102,11 +102,7 @@ export function ProxmoxRemotesPage() { const clusterType = config.type === 'pve' ? 've' : 'pbs'; const { hostname, port } = parseRemoteUrl(config.url, config.type); - // Edit operation requires remove-then-add since backend doesn't support update. - // If add fails after remove, the remote will be lost - this is a known limitation - // until backend supports atomic update operations. - await removeProxmoxCluster(config.id); - await addProxmoxCluster( + await updateProxmoxCluster( config.id, config.name, clusterType as ClusterType, @@ -139,15 +135,11 @@ export function ProxmoxRemotesPage() { const handleConnectRemote = async (remote: RemoteInfo) => { try { toast.info(`Testing connection to ${remote.name}...`); - const result = await getProxmoxCluster(remote.id); - if (result !== null) { - toast.success(`Connected to ${remote.name}`); - setRemotes((prev) => - prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) - ); - } else { - toast.error(`Cluster ${remote.name} not found`); - } + await pingProxmoxCluster(remote.id); + toast.success(`Connected to ${remote.name}`); + setRemotes((prev) => + prev.map((r) => (r.id === remote.id ? { ...r, status: 'connected' } : r)) + ); } catch (err) { console.error('Failed to connect remote:', err); toast.error('Connection failed: ' + String(err));