From 4f1f77e0aeb0a4042eceeb4cd52ee23ec14fd1d3 Mon Sep 17 00:00:00 2001
From: Pulse Monitor <pulse@pulse-monitor.dev>
Date: Thu, 28 Aug 2025 11:56:02 +0000
Subject: [PATCH] fix: improve error handling for node connection tests
 (addresses #362, #363)

- Handle standalone nodes properly in diagnostics (fixes #362)
  - Try /nodes endpoint first which works for both clustered and standalone
  - Make cluster status optional, not required for connection success
  - Set cluster node count to 1 for standalone nodes

- Fix test connection UI showing success styling on errors (fixes #363)
  - Return proper HTTP error status when test-config endpoint fails
  - Clean up error messages to remove "API request failed: XXX" prefix
  - Add debug logging to trace test result states
  - Ensure consistent error status codes from all test endpoints

Both issues reported connection problems - standalone nodes failing to connect
and error messages appearing with success styling. These fixes ensure proper
handling of both scenarios.
---
 .../src/components/Settings/NodeModal.tsx     | 21 +++++++++--
 internal/api/config_handlers.go               |  8 +++++
 internal/api/diagnostics.go                   | 36 ++++++++++++-------
 3 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/frontend-modern/src/components/Settings/NodeModal.tsx b/frontend-modern/src/components/Settings/NodeModal.tsx
index 0cd1e1ff7..4c3ebb51b 100644
--- a/frontend-modern/src/components/Settings/NodeModal.tsx
+++ b/frontend-modern/src/components/Settings/NodeModal.tsx
@@ -205,9 +205,15 @@ export const NodeModal: Component<NodeModalProps> = (props) => {
             message: result.message || 'Connection successful'
           });
         } catch (error) {
+          console.error('Test existing node error:', error);
+          let errorMessage = 'Connection failed';
+          if (error instanceof Error) {
+            // Remove "API request failed: XXX " prefix if present
+            errorMessage = error.message.replace(/^API request failed: \d{3}\s*/, '');
+          }
           setTestResult({
             status: 'error',
-            message: error instanceof Error ? error.message : 'Connection failed'
+            message: errorMessage
           });
         } finally {
           setIsTesting(false);
@@ -261,9 +267,15 @@ export const NodeModal: Component<NodeModalProps> = (props) => {
         isCluster: result.isCluster
       });
     } catch (error) {
+      console.error('Test connection error:', error);
+      let errorMessage = 'Connection failed';
+      if (error instanceof Error) {
+        // Remove "API request failed: XXX " prefix if present
+        errorMessage = error.message.replace(/^API request failed: \d{3}\s*/, '');
+      }
       setTestResult({
         status: 'error',
-        message: error instanceof Error ? error.message : 'Connection failed'
+        message: errorMessage
       });
     } finally {
       setIsTesting(false);
@@ -1256,6 +1268,11 @@ export const NodeModal: Component<NodeModalProps> = (props) => {
                 
                 {/* Test Result */}
                 <Show when={testResult()}>
+                  {(() => {
+                    const result = testResult();
+                    console.log('Test result display:', { status: result?.status, message: result?.message });
+                    return null;
+                  })()}
                   <div class={`mx-6 p-3 rounded-lg text-sm ${
                     testResult()?.status === 'success' 
                       ? 'bg-green-50 dark:bg-green-900/20 border border-green-200 dark:border-green-800 text-green-800 dark:text-green-200'
diff --git a/internal/api/config_handlers.go b/internal/api/config_handlers.go
index a49f545f6..e62c1dca6 100644
--- a/internal/api/config_handlers.go
+++ b/internal/api/config_handlers.go
@@ -1444,7 +1444,11 @@ func (h *ConfigHandlers) HandleTestNodeConfig(w http.ResponseWriter, r *http.Req
 		return
 	}
 
+	// Return appropriate HTTP status based on test result
 	w.Header().Set("Content-Type", "application/json")
+	if testResult["status"] == "error" {
+		w.WriteHeader(http.StatusBadRequest)
+	}
 	json.NewEncoder(w).Encode(testResult)
 }
 
@@ -1561,7 +1565,11 @@ func (h *ConfigHandlers) HandleTestNode(w http.ResponseWriter, r *http.Request)
 		}
 	}
 
+	// Return appropriate HTTP status based on test result
 	w.Header().Set("Content-Type", "application/json")
+	if testResult["status"] == "error" {
+		w.WriteHeader(http.StatusBadRequest)
+	}
 	json.NewEncoder(w).Encode(testResult)
 }
 
diff --git a/internal/api/diagnostics.go b/internal/api/diagnostics.go
index aa8af29b0..6591fa305 100644
--- a/internal/api/diagnostics.go
+++ b/internal/api/diagnostics.go
@@ -156,32 +156,42 @@ func (r *Router) handleDiagnostics(w http.ResponseWriter, req *http.Request) {
 				nodeDiag.Connected = false
 				nodeDiag.Error = err.Error()
 			} else {
-				// Try to get cluster status
-				if clusterStatus, err := client.GetClusterStatus(ctx); err != nil {
+				// Try to get nodes first (this should work for both clustered and standalone)
+				nodes, err := client.GetNodes(ctx)
+				if err != nil {
 					nodeDiag.Connected = false
-					nodeDiag.Error = "Connection established but cluster status failed: " + err.Error()
+					nodeDiag.Error = "Failed to connect to Proxmox API: " + err.Error()
 				} else {
 					nodeDiag.Connected = true
-					nodeDiag.ClusterInfo = &ClusterInfo{
-						Nodes: len(clusterStatus),
-					}
 					
-					// Get node details
-					if nodes, err := client.GetNodes(ctx); err == nil && len(nodes) > 0 {
+					// Set node details
+					if len(nodes) > 0 {
 						nodeDiag.Details = &NodeDetails{
 							NodeCount: len(nodes),
 						}
 						
 						// Get version from first node
-						if len(nodes) > 0 {
-							if status, err := client.GetNodeStatus(ctx, nodes[0].Node); err == nil && status != nil {
-								if status.PVEVersion != "" {
-									nodeDiag.Details.Version = status.PVEVersion
-								}
+						if status, err := client.GetNodeStatus(ctx, nodes[0].Node); err == nil && status != nil {
+							if status.PVEVersion != "" {
+								nodeDiag.Details.Version = status.PVEVersion
 							}
 						}
 					}
 					
+					// Try to get cluster status (this may fail for standalone nodes, which is OK)
+					if clusterStatus, err := client.GetClusterStatus(ctx); err == nil {
+						nodeDiag.ClusterInfo = &ClusterInfo{
+							Nodes: len(clusterStatus),
+						}
+					} else {
+						// Standalone node or cluster status not available
+						// This is not an error - standalone nodes don't have cluster status
+						log.Debug().Str("node", node.Name).Msg("Cluster status not available (likely standalone node)")
+						nodeDiag.ClusterInfo = &ClusterInfo{
+							Nodes: 1, // Standalone node
+						}
+					}
+					
 					// Run VM disk monitoring check
 					nodeDiag.VMDiskCheck = r.checkVMDiskMonitoring(ctx, client, node.Name)
 				}