fix(proxmox): address PR review findings
Some checks failed
Test / frontend-tests (pull_request) Successful in 1m38s
Test / frontend-typecheck (pull_request) Successful in 1m46s
PR Review Automation / review (pull_request) Successful in 5m37s
Test / rust-fmt-check (pull_request) Failing after 12m26s
Test / rust-clippy (pull_request) Successful in 13m43s
Test / rust-tests (pull_request) Successful in 16m4s

- 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
This commit is contained in:
Shaun Arman 2026-06-21 15:48:09 -05:00
parent 671ee51626
commit 58b4d59e6d
5 changed files with 29 additions and 69 deletions

View File

@ -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 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(""); let node_name = obj.get("node").and_then(|v| v.as_str()).unwrap_or("");
normalized.insert( // Avoid double-slash when cluster/resources omits "node" for shared storage
"id".to_string(), let storage_id = if node_name.is_empty() {
serde_json::Value::String(format!( format!("storage/{}", storage_name)
"storage/{}/{}", } else {
node_name, storage_name format!("storage/{}/{}", node_name, storage_name)
)), };
); normalized.insert("id".to_string(), serde_json::Value::String(storage_id));
normalized.insert( normalized.insert(
"storage".to_string(), "storage".to_string(),
serde_json::Value::String(storage_name.to_string()), serde_json::Value::String(storage_name.to_string()),

View File

@ -80,6 +80,8 @@ pub async fn list_firewall_rules(
} }
/// Add firewall rule — uses correct PVE API field names (proto, enable, dest). /// 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( pub async fn add_rule(
client: &crate::proxmox::client::ProxmoxClient, client: &crate::proxmox::client::ProxmoxClient,
node: &str, node: &str,

View File

@ -122,6 +122,8 @@ export function VMList({
})); }));
}, [rawVms]); }, [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) => { const handleVMAction = useCallback(async (vm: VMInfo, action: string) => {
if (!clusterId) { if (!clusterId) {
toast.error('No cluster selected'); toast.error('No cluster selected');
@ -746,6 +748,8 @@ function MigrationDialog({
<Button variant="outline" onClick={onClose}> <Button variant="outline" onClick={onClose}>
Cancel Cancel
</Button> </Button>
{/* Disabled when: no target node typed/selected,
OR same-cluster migration with no enumerated nodes to choose from */}
<Button <Button
onClick={onSubmit} onClick={onSubmit}
disabled={!targetNode || (!isCrossCluster && availableTargets.length === 0)} disabled={!targetNode || (!isCrossCluster && availableTargets.length === 0)}

View File

@ -80,26 +80,14 @@ export function ProxmoxBackupPage() {
}, [selectedClusterId, loadJobs]); }, [selectedClusterId, loadJobs]);
const handleNewJob = () => { const handleNewJob = () => {
setJobName(''); toast.warning(
setJobNode(''); 'Backup job creation requires additional backend implementation (POST cluster/backup) and is not yet available.',
setJobSchedule(''); );
setJobVms('');
setShowNewJobDialog(true);
}; };
const handleSubmitNewJob = async () => { const handleSubmitNewJob = async () => {
if (!jobName || !jobNode || !jobSchedule) { toast.warning('Backup job creation is not yet available.');
toast.error('Job name, node, and schedule are required');
return;
}
try {
toast.info(`Creating backup job ${jobName} - implementation pending`);
setShowNewJobDialog(false); setShowNewJobDialog(false);
} catch (error) {
console.error('Failed to create backup job:', error);
toast.error(`Failed to create backup job: ${error}`);
}
}; };
if (clusters.length === 0 && !isLoading) { if (clusters.length === 0 && !isLoading) {

View File

@ -17,7 +17,7 @@ export function ProxmoxNetworkPage() {
const [loading, setLoading] = useState(false); const [loading, setLoading] = useState(false);
const [error, setError] = useState<string | null>(null); const [error, setError] = useState<string | null>(null);
const [showAddDialog, setShowAddDialog] = useState(false); const [showAddDialog, setShowAddDialog] = useState(false);
const [editingInterface, setEditingInterface] = useState<NetworkInterface | null>(null); const [editingInterface] = useState<NetworkInterface | null>(null);
// Form state // Form state
const [ifaceName, setIfaceName] = useState(''); const [ifaceName, setIfaceName] = useState('');
@ -52,58 +52,24 @@ export function ProxmoxNetworkPage() {
.catch(console.error); .catch(console.error);
}, [loadInterfaces, nodeId]); }, [loadInterfaces, nodeId]);
const NOT_IMPLEMENTED_MSG =
'Network interface management requires additional backend implementation (POST/PUT/DELETE nodes/{node}/network) and is not yet available.';
const handleAddInterface = () => { const handleAddInterface = () => {
setEditingInterface(null); toast.warning(NOT_IMPLEMENTED_MSG);
setIfaceName('');
setIfaceType('eth');
setAddress('');
setNetmask('');
setGateway('');
setActive(true);
setShowAddDialog(true);
}; };
const handleEditInterface = (iface: NetworkInterface) => { const handleEditInterface = (_iface: NetworkInterface) => {
setEditingInterface(iface); toast.warning(NOT_IMPLEMENTED_MSG);
setIfaceName(iface.iface);
setIfaceType(iface.type);
setAddress(iface.address || '');
setNetmask(iface.netmask || '');
setGateway(iface.gateway || '');
setActive(iface.active);
setShowAddDialog(true);
}; };
const handleSubmit = async () => { const handleSubmit = async () => {
if (!ifaceName || !ifaceType) { toast.warning(NOT_IMPLEMENTED_MSG);
toast.error('Interface name and type are required');
return;
}
try {
if (editingInterface) {
toast.info(`Updating interface ${ifaceName} - implementation pending`);
} else {
toast.info(`Creating interface ${ifaceName} - implementation pending`);
}
setShowAddDialog(false); setShowAddDialog(false);
} catch (error) {
console.error('Failed to save interface:', error);
toast.error(`Failed to save interface: ${error}`);
}
}; };
const handleDeleteInterface = async (iface: NetworkInterface) => { const handleDeleteInterface = async (_iface: NetworkInterface) => {
if (!confirm(`Are you sure you want to delete interface ${iface.iface}?`)) { toast.warning(NOT_IMPLEMENTED_MSG);
return;
}
try {
toast.info(`Deleting interface ${iface.iface} - implementation pending`);
} catch (error) {
console.error('Failed to delete interface:', error);
toast.error(`Failed to delete interface: ${error}`);
}
}; };
return ( return (