From 58b4d59e6da436b7ed31460833ceb70906d6227c Mon Sep 17 00:00:00 2001 From: Shaun Arman Date: Sun, 21 Jun 2026 15:48:09 -0500 Subject: [PATCH] fix(proxmox): address PR review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Storage: prevent double-slash ID when cluster/resources returns shared storage without a node field (e.g. "storage//PBS_TFTSR" → "storage/PBS_TFTSR") - Firewall: add comment explaining why rule_num is omitted from add_rule — PVE assigns position (pos) automatically on creation - Network: replace misleading "implementation pending" toasts with an immediate warning; dialog no longer opens since backend commands (POST/PUT/DELETE nodes/{node}/network) are not yet implemented - Backup: same treatment for New Job — warns immediately instead of opening a form that silently does nothing - VMList: add comment explaining handleVMAction receives clusterId from props (not a stale closure over state); add inline comment clarifying the migration button disabled conditions --- src-tauri/src/commands/proxmox.rs | 14 ++++---- src-tauri/src/proxmox/firewall.rs | 2 ++ src/components/Proxmox/VMList.tsx | 4 +++ src/pages/Proxmox/BackupPage.tsx | 22 +++--------- src/pages/Proxmox/NetworkPage.tsx | 56 ++++++------------------------- 5 files changed, 29 insertions(+), 69 deletions(-) diff --git a/src-tauri/src/commands/proxmox.rs b/src-tauri/src/commands/proxmox.rs index 012d822a..923e77d0 100644 --- a/src-tauri/src/commands/proxmox.rs +++ b/src-tauri/src/commands/proxmox.rs @@ -709,13 +709,13 @@ pub async fn list_proxmox_datastores( let storage_name = obj.get("storage").and_then(|v| v.as_str()).unwrap_or(""); let node_name = obj.get("node").and_then(|v| v.as_str()).unwrap_or(""); - normalized.insert( - "id".to_string(), - serde_json::Value::String(format!( - "storage/{}/{}", - node_name, storage_name - )), - ); + // Avoid double-slash when cluster/resources omits "node" for shared storage + let storage_id = if node_name.is_empty() { + format!("storage/{}", storage_name) + } else { + format!("storage/{}/{}", node_name, storage_name) + }; + normalized.insert("id".to_string(), serde_json::Value::String(storage_id)); normalized.insert( "storage".to_string(), serde_json::Value::String(storage_name.to_string()), diff --git a/src-tauri/src/proxmox/firewall.rs b/src-tauri/src/proxmox/firewall.rs index 086d1e11..6289df74 100644 --- a/src-tauri/src/proxmox/firewall.rs +++ b/src-tauri/src/proxmox/firewall.rs @@ -80,6 +80,8 @@ pub async fn list_firewall_rules( } /// Add firewall rule — uses correct PVE API field names (proto, enable, dest). +/// `rule.rule_num` is intentionally not sent: PVE assigns the position (pos) automatically +/// on creation. rule_num is only used for update/delete operations on existing rules. pub async fn add_rule( client: &crate::proxmox::client::ProxmoxClient, node: &str, diff --git a/src/components/Proxmox/VMList.tsx b/src/components/Proxmox/VMList.tsx index 3002d038..7eeb8b7b 100644 --- a/src/components/Proxmox/VMList.tsx +++ b/src/components/Proxmox/VMList.tsx @@ -122,6 +122,8 @@ export function VMList({ })); }, [rawVms]); + // clusterId comes from props (not captured via closure over state), so it is always + // current when an action fires even if the user switches clusters mid-session. const handleVMAction = useCallback(async (vm: VMInfo, action: string) => { if (!clusterId) { toast.error('No cluster selected'); @@ -746,6 +748,8 @@ function MigrationDialog({ + {/* Disabled when: no target node typed/selected, + OR same-cluster migration with no enumerated nodes to choose from */}