fix: Implement Phase 2 major fixes

Database:
- Add missing indexes for api_key_hash, status, metrics queries
- New migration: 005_add_missing_indexes.sql

Server:
- Fix WebSocket Ping/Pong protocol (RFC 6455 compliance)
- Use separate channel for Pong responses

Agent:
- Replace format!() path construction with PathBuf::join()
- Replace todo!() macros with proper errors for macOS support

Dashboard:
- Fix duplicate filter values in Agents page (__unassigned__ sentinel)
- Add onError handlers to all mutations in Agents, Clients, Sites pages

All changes reviewed and approved.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

projects/msp-tools/guru-rmm/agent/src/main.rs

@@ -224,8 +224,11 @@ async fn install_service(
     #[cfg(target_os = "macos")]
     {
         let _ = (server_url, api_key, skip_legacy_check); // Suppress unused warnings
-        info!("Installing GuruRMM Agent as launchd service...");
-        todo!("macOS launchd service installation not yet implemented");
+        return Err(anyhow::anyhow!(
+            "macOS launchd service installation is not yet implemented.\n\
+             For now, you can run the agent manually or create a launchd plist.\n\
+             See: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLaunchdJobs.html"
+        ));
     }
 }
 
@@ -504,7 +507,10 @@ async fn uninstall_service() -> Result<()> {
 
     #[cfg(target_os = "macos")]
     {
-        todo!("macOS service uninstallation not yet implemented");
+        return Err(anyhow::anyhow!(
+            "macOS launchd service uninstallation is not yet implemented.\n\
+             If you created a launchd plist manually, remove it from ~/Library/LaunchAgents/ or /Library/LaunchDaemons/"
+        ));
     }
 }
 
@@ -593,7 +599,10 @@ async fn start_service() -> Result<()> {
 
     #[cfg(target_os = "macos")]
     {
-        todo!("macOS service start not yet implemented");
+        return Err(anyhow::anyhow!(
+            "macOS launchd service start is not yet implemented.\n\
+             If you created a launchd plist manually, use: launchctl load <plist-path>"
+        ));
     }
 }
 
@@ -626,7 +635,10 @@ async fn stop_service() -> Result<()> {
 
     #[cfg(target_os = "macos")]
     {
-        todo!("macOS service stop not yet implemented");
+        return Err(anyhow::anyhow!(
+            "macOS launchd service stop is not yet implemented.\n\
+             If you created a launchd plist manually, use: launchctl unload <plist-path>"
+        ));
     }
 }
 

projects/msp-tools/guru-rmm/agent/src/service.rs

@@ -90,7 +90,7 @@ pub mod windows {
             .context("Failed to set StartPending status")?;
 
         // Determine config path
-        let config_path = PathBuf::from(format!(r"{}\\agent.toml", CONFIG_DIR));
+        let config_path = PathBuf::from(CONFIG_DIR).join("agent.toml");
 
         // Create the tokio runtime for the agent
         let runtime = tokio::runtime::Runtime::new().context("Failed to create tokio runtime")?;
@@ -331,8 +331,8 @@ pub mod windows {
         let current_exe =
             std::env::current_exe().context("Failed to get current executable path")?;
 
-        let binary_dest = PathBuf::from(format!(r"{}\\gururmm-agent.exe", INSTALL_DIR));
-        let config_dest = PathBuf::from(format!(r"{}\\agent.toml", CONFIG_DIR));
+        let binary_dest = PathBuf::from(INSTALL_DIR).join("gururmm-agent.exe");
+        let config_dest = PathBuf::from(CONFIG_DIR).join("agent.toml");
 
         // Create directories
         info!("Creating directories...");
@@ -490,7 +490,7 @@ pub mod windows {
     pub fn uninstall() -> Result<()> {
         info!("Uninstalling GuruRMM Agent...");
 
-        let binary_path = PathBuf::from(format!(r"{}\\gururmm-agent.exe", INSTALL_DIR));
+        let binary_path = PathBuf::from(INSTALL_DIR).join("gururmm-agent.exe");
 
         // Open the service manager
         let manager = ServiceManager::local_computer(
@@ -685,8 +685,8 @@ pub mod windows {
 
         // Get the current executable path
         let current_exe = std::env::current_exe()?;
-        let binary_dest = PathBuf::from(format!(r"{}\\gururmm-agent.exe", INSTALL_DIR));
-        let config_dest = PathBuf::from(format!(r"{}\\agent.toml", CONFIG_DIR));
+        let binary_dest = PathBuf::from(INSTALL_DIR).join("gururmm-agent.exe");
+        let config_dest = PathBuf::from(CONFIG_DIR).join("agent.toml");
 
         // Create directories
         std::fs::create_dir_all(INSTALL_DIR)?;
@@ -727,7 +727,7 @@ pub mod windows {
     pub fn uninstall() -> Result<()> {
         use std::path::PathBuf;
 
-        let binary_path = PathBuf::from(format!(r"{}\\gururmm-agent.exe", INSTALL_DIR));
+        let binary_path = PathBuf::from(INSTALL_DIR).join("gururmm-agent.exe");
 
         println!("** To uninstall legacy service, use NSSM:");
         println!("   nssm stop {}", SERVICE_NAME);

projects/msp-tools/guru-rmm/dashboard/src/pages/Agents.tsx

@@ -101,6 +101,10 @@ export function Agents() {
       queryClient.invalidateQueries({ queryKey: ["agents"] });
       setDeleteConfirm(null);
     },
+    onError: (error: Error) => {
+      console.error("Failed to delete agent:", error);
+      alert(`Failed to delete agent: ${error.message}`);
+    },
   });
 
   const moveMutation = useMutation({
@@ -111,6 +115,10 @@ export function Agents() {
       queryClient.invalidateQueries({ queryKey: ["sites"] });
       setMovingAgent(null);
     },
+    onError: (error: Error) => {
+      console.error("Failed to move agent:", error);
+      alert(`Failed to move agent: ${error.message}`);
+    },
   });
 
   // Get unique clients from agents
@@ -124,7 +132,17 @@ export function Agents() {
       (agent.client_name && agent.client_name.toLowerCase().includes(search.toLowerCase())) ||
       (agent.site_name && agent.site_name.toLowerCase().includes(search.toLowerCase()));
 
-    const matchesClient = !filterClient || agent.client_name === filterClient;
+    // Handle special __unassigned__ filter value
+    let matchesClient: boolean;
+    if (filterClient === "__unassigned__") {
+      // Show only agents without a site/client assignment
+      matchesClient = !agent.site_id;
+    } else if (filterClient) {
+      matchesClient = agent.client_name === filterClient;
+    } else {
+      matchesClient = true;
+    }
+
     const matchesSite = !filterSite || agent.site_id === filterSite;
 
     return matchesSearch && matchesClient && matchesSite;
@@ -173,7 +191,7 @@ export function Agents() {
           className="px-3 py-2 rounded-md border border-[hsl(var(--border))] bg-[hsl(var(--background))] text-sm"
         >
           <option value="">All Clients</option>
-          <option value="">-- Unassigned --</option>
+          <option value="__unassigned__">Unassigned Only</option>
           {clients.map((client) => (
             <option key={client} value={client || ""}>
               {client}

projects/msp-tools/guru-rmm/dashboard/src/pages/Clients.tsx

@@ -101,6 +101,10 @@ export function Clients() {
       queryClient.invalidateQueries({ queryKey: ["clients"] });
       setShowModal(false);
     },
+    onError: (error: Error) => {
+      console.error("Failed to create client:", error);
+      alert(`Failed to create client: ${error.message}`);
+    },
   });
 
   const updateMutation = useMutation({
@@ -111,6 +115,10 @@ export function Clients() {
       setShowModal(false);
       setEditingClient(undefined);
     },
+    onError: (error: Error) => {
+      console.error("Failed to update client:", error);
+      alert(`Failed to update client: ${error.message}`);
+    },
   });
 
   const deleteMutation = useMutation({
@@ -119,6 +127,10 @@ export function Clients() {
       queryClient.invalidateQueries({ queryKey: ["clients"] });
       setDeleteConfirm(null);
     },
+    onError: (error: Error) => {
+      console.error("Failed to delete client:", error);
+      alert(`Failed to delete client: ${error.message}`);
+    },
   });
 
   const handleSave = (data: ClientFormData) => {

projects/msp-tools/guru-rmm/dashboard/src/pages/Sites.tsx

@@ -195,6 +195,10 @@ export function Sites() {
       const data = response.data as CreateSiteResponse;
       setNewSiteApiKey({ apiKey: data.api_key, siteCode: data.site.site_code });
     },
+    onError: (error: Error) => {
+      console.error("Failed to create site:", error);
+      alert(`Failed to create site: ${error.message}`);
+    },
   });
 
   const updateMutation = useMutation({
@@ -205,6 +209,10 @@ export function Sites() {
       setShowModal(false);
       setEditingSite(undefined);
     },
+    onError: (error: Error) => {
+      console.error("Failed to update site:", error);
+      alert(`Failed to update site: ${error.message}`);
+    },
   });
 
   const deleteMutation = useMutation({
@@ -214,6 +222,10 @@ export function Sites() {
       queryClient.invalidateQueries({ queryKey: ["clients"] });
       setDeleteConfirm(null);
     },
+    onError: (error: Error) => {
+      console.error("Failed to delete site:", error);
+      alert(`Failed to delete site: ${error.message}`);
+    },
   });
 
   const regenerateKeyMutation = useMutation({
@@ -226,6 +238,11 @@ export function Sites() {
       });
       setRegeneratingKey(null);
     },
+    onError: (error: Error) => {
+      console.error("Failed to regenerate API key:", error);
+      alert(`Failed to regenerate API key: ${error.message}`);
+      setRegeneratingKey(null);
+    },
   });
 
   const handleSave = (data: SiteFormData) => {

projects/msp-tools/guru-rmm/server/migrations/005_add_missing_indexes.sql

@@ -0,0 +1,26 @@
+-- Migration: Add missing indexes for performance
+-- These indexes were identified during code review as missing
+-- Date: 2026-01-20
+
+-- Index for agent API key lookups (authentication)
+-- Supports legacy authentication where agents have their own api_key_hash
+CREATE INDEX IF NOT EXISTS idx_agents_api_key_hash ON agents(api_key_hash);
+
+-- Index for site API key lookups
+-- Note: idx_sites_api_key already exists from migration 002, but using consistent naming
+-- This is a no-op if the index already exists
+CREATE INDEX IF NOT EXISTS idx_sites_api_key_hash ON sites(api_key_hash);
+
+-- Index for command status queries (find all pending/running commands)
+-- Note: idx_commands_agent_status exists for (agent_id, status)
+-- This index is for querying by status alone (e.g., find all pending commands)
+CREATE INDEX IF NOT EXISTS idx_commands_status ON commands(status);
+
+-- Index for metrics time-range queries
+-- Note: idx_metrics_agent_time already exists for (agent_id, timestamp DESC)
+-- This is equivalent - creating with requested name for compatibility
+CREATE INDEX IF NOT EXISTS idx_metrics_agent_timestamp ON metrics(agent_id, timestamp DESC);
+
+-- Index for finding online agents quickly with last_seen ordering
+-- Allows efficient queries like: WHERE status = 'online' ORDER BY last_seen DESC
+CREATE INDEX IF NOT EXISTS idx_agents_status_last_seen ON agents(status, last_seen DESC);

projects/msp-tools/guru-rmm/server/src/ws/mod.rs

@@ -7,7 +7,6 @@
 //! - Watchdog event handling
 
 use std::collections::HashMap;
-use std::sync::Arc;
 
 use axum::{
     extract::{
@@ -18,7 +17,7 @@ use axum::{
 };
 use futures_util::{SinkExt, StreamExt};
 use serde::{Deserialize, Serialize};
-use tokio::sync::{mpsc, RwLock};
+use tokio::sync::mpsc;
 use tracing::{debug, error, info, warn};
 use uuid::Uuid;
 
@@ -253,9 +252,13 @@ pub async fn ws_handler(ws: WebSocketUpgrade, State(state): State<AppState>) ->
 async fn handle_socket(socket: WebSocket, state: AppState) {
     let (mut sender, mut receiver) = socket.split();
 
-    // Create channel for outgoing messages
+    // Create channel for outgoing protocol messages (ServerMessage)
     let (tx, mut rx) = mpsc::channel::<ServerMessage>(100);
 
+    // Create separate channel for raw WebSocket frames (Pong responses)
+    // This allows proper WebSocket protocol compliance without changing the public API
+    let (pong_tx, mut pong_rx) = mpsc::channel::<Vec<u8>>(16);
+
     // Wait for authentication message
     let auth_result = match authenticate(&mut receiver, &mut sender, &state).await {
         Ok(result) => {
@@ -358,12 +361,26 @@ async fn handle_socket(socket: WebSocket, state: AppState) {
     let agent_id = auth_result.agent_id;
 
     // Spawn task to forward outgoing messages
+    // Handles both protocol messages (ServerMessage) and raw Pong frames
     let send_task = tokio::spawn(async move {
-        while let Some(msg) = rx.recv().await {
-            if let Ok(json) = serde_json::to_string(&msg) {
-                if sender.send(Message::Text(json)).await.is_err() {
-                    break;
+        loop {
+            tokio::select! {
+                // Handle protocol messages (ServerMessage -> JSON text)
+                Some(msg) = rx.recv() => {
+                    if let Ok(json) = serde_json::to_string(&msg) {
+                        if sender.send(Message::Text(json)).await.is_err() {
+                            break;
+                        }
+                    }
                 }
+                // Handle Pong responses (WebSocket protocol compliance)
+                Some(data) = pong_rx.recv() => {
+                    if sender.send(Message::Pong(data)).await.is_err() {
+                        break;
+                    }
+                }
+                // Both channels closed
+                else => break,
             }
         }
     });
@@ -377,11 +394,10 @@ async fn handle_socket(socket: WebSocket, state: AppState) {
                 }
             }
             Ok(Message::Ping(data)) => {
-                if tx
-                    .send(ServerMessage::Ack { message_id: None })
-                    .await
-                    .is_err()
-                {
+                // WebSocket protocol requires Pong response with same payload
+                // Send via pong channel to the send task
+                if pong_tx.send(data).await.is_err() {
+                    warn!("Failed to send Pong response for agent {}", agent_id);
                     break;
                 }
             }