From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 42D911FF191 for ; Tue, 7 Oct 2025 10:42:28 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E1474137CA; Tue, 7 Oct 2025 10:42:29 +0200 (CEST) Mime-Version: 1.0 Date: Tue, 07 Oct 2025 10:42:26 +0200 Message-Id: Cc: "pve-devel" To: "Proxmox VE development discussion" X-Mailer: aerc 0.20.0 References: <20251006133308.119141-1-n.frey@proxmox.com> <20251006133308.119141-2-n.frey@proxmox.com> In-Reply-To: <20251006133308.119141-2-n.frey@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1759826517087 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.056 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" some small comments in-line On Mon Oct 6, 2025 at 3:33 PM CEST, Nicolas Frey wrote: > Adds snapshot and manual backup shortcut to VM/CT right-click context > menu. > > On the previous iteration (v2), the snapshot feature availablity was > checked on menu load. It is now called only when the snapshot button > is clicked, preventing delays in loading the context menu. Testing > with simulated latency (with setTimeout) showed better UX by > displaying an error on click rather than preloading the check. > > Previously, users couldn't distinguish between a slow API and the > feature being disabled. The explicit check provides clear feedback. > The 'disabled' property remains bound to VM.Snapshot to disable > the option when snapshots aren't allowed anyway. > > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=6209 > Signed-off-by: Nicolas Frey > --- > www/manager6/lxc/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++ > www/manager6/qemu/CmdMenu.js | 46 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 92 insertions(+) > > diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js > index cd60c967..f0e8f700 100644 > --- a/www/manager6/lxc/CmdMenu.js > +++ b/www/manager6/lxc/CmdMenu.js > @@ -134,6 +134,52 @@ Ext.define('PVE.lxc.CmdMenu', { > }, > }, > { xtype: 'menuseparator' }, > + { > + text: gettext('Take Snapshot'), > + iconCls: 'fa fa-fw fa-history', > + disabled: !caps.vms['VM.Snapshot'], > + handler: function () { > + Proxmox.Utils.API2Request({ > + url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`, > + params: { feature: 'snapshot' }, > + method: 'GET', > + success: function (response) { nit: you are mixing arrow style functions with anonymous functions here, imo this should also just be: (response) => { [..] } > + if (!response.result.data.hasFeature) { > + Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.') nit: did you run `make tidy`? for me it cleans the line above up to: Ext.Msg.alert( gettext('Error'), 'Cannot take snapshot: Feature not enabled.', ); > + return; > + } > + Ext.create('PVE.window.Snapshot', { > + nodename: info.node, > + vmid: info.vmid, > + vmname: info.name, > + viewonly: false, > + type: info.type, > + isCreate: true, > + submitText: gettext('Take Snapshot'), > + autoShow: true, > + running: running, > + }); > + }, > + failure: (response, opts) => > + Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + }); > + }, > + }, > + { > + text: gettext('Backup now'), > + iconCls: 'fa fa-fw fa-floppy-o', > + disabled: !caps.vms['VM.Backup'], > + handler: function () { see comment about arrow function above > + Ext.create('PVE.window.Backup', { > + nodename: info.node, > + vmid: info.vmid, > + vmtype: info.type, > + vmname: info.name, > + autoShow: true, > + }); > + }, > + }, > + { xtype: 'menuseparator' }, > { > text: gettext('Console'), > iconCls: 'fa fa-fw fa-terminal', > diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js > index adf64672..3e51be8f 100644 > --- a/www/manager6/qemu/CmdMenu.js > +++ b/www/manager6/qemu/CmdMenu.js > @@ -169,6 +169,52 @@ Ext.define('PVE.qemu.CmdMenu', { > }, > }, > { xtype: 'menuseparator' }, > + { > + text: gettext('Take Snapshot'), > + iconCls: 'fa fa-fw fa-history', > + disabled: !caps.vms['VM.Snapshot'], > + handler: function () { > + Proxmox.Utils.API2Request({ > + url: `/nodes/${info.node}/${info.type}/${info.vmid}/feature`, > + params: { feature: 'snapshot' }, > + method: 'GET', > + success: function (response) { see comment about arrow function above > + if (!response.result.data.hasFeature) { > + Ext.Msg.alert(gettext('Error'), 'Cannot take snapshot: Feature not enabled.') see comment about `make tidy` above > + return; > + } > + Ext.create('PVE.window.Snapshot', { > + nodename: info.node, > + vmid: info.vmid, > + vmname: info.name, > + viewonly: false, > + type: info.type, > + isCreate: true, > + submitText: gettext('Take Snapshot'), > + autoShow: true, > + running: running, > + }); > + }, > + failure: (response, opts) => > + Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + }); > + }, > + }, > + { > + text: gettext('Backup now'), > + iconCls: 'fa fa-fw fa-floppy-o', > + disabled: !caps.vms['VM.Backup'], > + handler: function () { see comment about arrow function above > + Ext.create('PVE.window.Backup', { > + nodename: info.node, > + vmid: info.vmid, > + vmtype: info.type, > + vmname: info.name, > + autoShow: true, > + }); > + }, > + }, > + { xtype: 'menuseparator' }, > { > text: gettext('Console'), > iconCls: 'fa fa-fw fa-terminal', one small thought: have you explored whether querying the features of the guest when opening the context menu and graying out the snapshot option is viable? imo that would be a nicer user experience, but the overhead of querying the backend there might be too much. other than that: Reviewed-by: Shannon Sterz _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel