Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu

[Nicolas Frey <n.frey@proxmox.com>]

thanks for the review!

On 10/7/25 10:42 AM, Shannon Sterz wrote:
> 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 <n.frey@proxmox.com>
>> ---
>>  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) => {
>     [..]
> }

will change them all to anonymous functions in a v4

> 
>> +                            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:

forgot to do so, thanks for pointing that out!

> 
>                                 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.
that was discussed (accidentally) off list but now has been re-sent by
Thomas [0]. That was the previous approach and it did seem like quite
a bit of overhead for just the context menu. IMO it is also better
experience to not have to wait until a button is enabled (as would be
the case with very high latency). I would rather wait/get an error
*after* clicking.

[0]
https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/T/#u

> 
> other than that:
> 
> Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel