fix: address PR review findings — race condition, real ping, atomic edit, listener cleanup

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

@@ -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<serde_json::Value, String> {
+    let client = get_proxmox_client_for_cluster(&cluster_id, &state).await?;
+    let client_guard = client.lock().await;
+
+    client_guard
+        .get::<serde_json::Value>("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<ClusterInfo, String> {
+    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"));
+    }
 }

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,

src/components/Proxmox/RemotesList.tsx

@@ -42,7 +42,6 @@ function ActionsMenu({
   const menuRef = useRef<HTMLDivElement>(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 (
     <div className="relative" ref={menuRef}>

src/lib/proxmoxClient.ts

@@ -40,6 +40,36 @@ export async function removeProxmoxCluster(id: string): Promise<void> {
   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<ClusterInfo> {
+  return await invoke<ClusterInfo>("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<any> {
+  return await invoke<any>("ping_proxmox_cluster", { clusterId });
+}
+
 /**
  * List all Proxmox clusters
  */

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));