From 64fc8089085033271ec6edec4ef9a43214e399ce Mon Sep 17 00:00:00 2001
From: Shaun Arman <shaun.arman@motorolasolutions.com>
Date: Sun, 21 Jun 2026 18:16:13 -0500
Subject: [PATCH] fix(proxmox): address PR review suggestions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Cores max aligned to backend limit (128→512)
- MigrationDialog shows loading state while node list fetches
- CreateVmDialog validates ISO format client-side on change, blocks
  submit and highlights field on invalid input
---
 src/components/Proxmox/CreateVmDialog.tsx | 28 +++++++++++++++++++----
 src/components/Proxmox/VMList.tsx         | 11 ++++++++-
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/src/components/Proxmox/CreateVmDialog.tsx b/src/components/Proxmox/CreateVmDialog.tsx
index f8c262fd..2a189226 100644
--- a/src/components/Proxmox/CreateVmDialog.tsx
+++ b/src/components/Proxmox/CreateVmDialog.tsx
@@ -40,6 +40,7 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
   const [diskSize, setDiskSize] = useState(20);
   const [netBridge, setNetBridge] = useState('vmbr0');
   const [iso, setIso] = useState('');
+  const [isoError, setIsoError] = useState('');
 
   useEffect(() => {
     if (!isOpen || !clusterId) return;
@@ -69,10 +70,23 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
       });
   }, [isOpen, clusterId]);
 
+  const ISO_RE = /^[a-zA-Z0-9_-]+:iso\/.+\.iso$/;
+
+  const validateIso = (value: string): string => {
+    if (!value) return '';
+    return ISO_RE.test(value) ? '' : "Must be in the format 'storage:iso/filename.iso'";
+  };
+
+  const handleIsoChange = (value: string) => {
+    setIso(value);
+    setIsoError(validateIso(value));
+  };
+
   const handleSubmit = async () => {
     if (!nodeId) { toast.error('Please select a target node'); return; }
     if (!name.trim()) { toast.error('VM name is required'); return; }
     if (vmid < 100 || vmid > 999999999) { toast.error('VMID must be between 100 and 999999999'); return; }
+    if (isoError) { toast.error(isoError); return; }
 
     setIsSubmitting(true);
     try {
@@ -109,6 +123,7 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
     setDiskSize(20);
     setNetBridge('vmbr0');
     setIso('');
+    setIsoError('');
     onClose();
   };
 
@@ -189,7 +204,7 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
                 id="vm-cores"
                 type="number"
                 min={1}
-                max={128}
+                max={512}
                 value={cores}
                 onChange={(e) => setCores(Number(e.target.value))}
               />
@@ -257,10 +272,15 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
             <Input
               id="vm-iso"
               value={iso}
-              onChange={(e) => setIso(e.target.value)}
+              onChange={(e) => handleIsoChange(e.target.value)}
               placeholder="local:iso/ubuntu-24.04.iso"
+              className={isoError ? 'border-red-500' : ''}
             />
-            <p className="text-xs text-muted-foreground">Format: storage:iso/filename.iso</p>
+            {isoError ? (
+              <p className="text-xs text-red-500">{isoError}</p>
+            ) : (
+              <p className="text-xs text-muted-foreground">Format: storage:iso/filename.iso</p>
+            )}
           </div>
         </div>
 
@@ -268,7 +288,7 @@ export function CreateVmDialog({ isOpen, clusterId, onClose, onCreated }: Create
           <Button variant="outline" onClick={handleClose} disabled={isSubmitting}>
             Cancel
           </Button>
-          <Button onClick={handleSubmit} disabled={isSubmitting || !nodeId || !name.trim()}>
+          <Button onClick={handleSubmit} disabled={isSubmitting || !nodeId || !name.trim() || !!isoError}>
             {isSubmitting ? 'Creating...' : 'Create VM'}
           </Button>
         </DialogFooter>
diff --git a/src/components/Proxmox/VMList.tsx b/src/components/Proxmox/VMList.tsx
index 36b3ff9d..f5b08b6c 100644
--- a/src/components/Proxmox/VMList.tsx
+++ b/src/components/Proxmox/VMList.tsx
@@ -104,6 +104,7 @@ export function VMList({
   const [onlineMigration, setOnlineMigration] = useState(true);
   const [maxDowntime, setMaxDowntime] = useState(30);
   const [clusterNodes, setClusterNodes] = useState<string[]>([]);
+  const [nodesLoading, setNodesLoading] = useState(false);
 
   const vms: VMInfo[] = React.useMemo(() => {
     return rawVms.map((vm) => ({
@@ -199,6 +200,7 @@ export function VMList({
   const handleMigrate = useCallback(async (vm: VMInfo) => {
     setMigrationVM(vm);
     setTargetCluster(clusterId);
+    setNodesLoading(true);
     try {
       const nodeData: { node?: string; status?: string }[] = await invoke('list_proxmox_nodes', { clusterId });
       const names = nodeData
@@ -212,6 +214,8 @@ export function VMList({
         .filter((node, idx, self) => self.indexOf(node) === idx && node !== vm.node);
       setClusterNodes(fallback);
       setTargetNode(fallback[0] || '');
+    } finally {
+      setNodesLoading(false);
     }
   }, [clusterId, vms]);
 
@@ -417,6 +421,7 @@ export function VMList({
         onClose={() => { setMigrationVM(null); setTargetNode(''); setTargetCluster(''); setClusterNodes([]); }}
         onSubmit={submitMigration}
         availableNodeNames={clusterNodes}
+        nodesLoading={nodesLoading}
         clusters={clusters}
         currentClusterId={clusterId}
         targetNode={targetNode}
@@ -607,6 +612,7 @@ interface MigrationDialogProps {
   onClose: () => void;
   onSubmit: () => void;
   availableNodeNames: string[];
+  nodesLoading: boolean;
   clusters: ClusterInfo[];
   currentClusterId: string;
   targetNode: string;
@@ -625,6 +631,7 @@ function MigrationDialog({
   onClose,
   onSubmit,
   availableNodeNames,
+  nodesLoading,
   clusters,
   currentClusterId,
   targetNode,
@@ -685,7 +692,9 @@ function MigrationDialog({
 
           <div className="space-y-2">
             <Label htmlFor="targetNode">Target Node</Label>
-            {isCrossCluster ? (
+            {nodesLoading ? (
+              <p className="text-sm text-muted-foreground animate-pulse">Loading nodes…</p>
+            ) : isCrossCluster ? (
               <>
                 <Input
                   id="targetNode"