* Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu
2025-10-07 8:42 ` Shannon Sterz
@ 2025-10-07 9:13 ` Shannon Sterz
2025-10-07 9:14 ` Nicolas Frey
2025-10-07 9:16 ` Thomas Lamprecht
2 siblings, 0 replies; 9+ messages in thread
From: Shannon Sterz @ 2025-10-07 9:13 UTC (permalink / raw)
To: Shannon Sterz, Proxmox VE development discussion; +Cc: pve-devel
On Tue Oct 7, 2025 at 10:42 AM CEST, 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) => {
> [..]
> }
>
>> + 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.
just saw thomas' review of v2 of this series, so nevermind that. sorry
for the noise.
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu
2025-10-07 8:42 ` Shannon Sterz
2025-10-07 9:13 ` Shannon Sterz
@ 2025-10-07 9:14 ` Nicolas Frey
2025-10-07 9:16 ` Thomas Lamprecht
2 siblings, 0 replies; 9+ messages in thread
From: Nicolas Frey @ 2025-10-07 9:14 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu
2025-10-07 8:42 ` Shannon Sterz
2025-10-07 9:13 ` Shannon Sterz
2025-10-07 9:14 ` Nicolas Frey
@ 2025-10-07 9:16 ` Thomas Lamprecht
2025-10-07 10:35 ` Nicolas Frey
2 siblings, 1 reply; 9+ messages in thread
From: Thomas Lamprecht @ 2025-10-07 9:16 UTC (permalink / raw)
To: Proxmox VE development discussion, Shannon Sterz, Nicolas Frey; +Cc: pve-devel
Am 07.10.25 um 10:42 schrieb Shannon Sterz:
> 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.
FWIW, Nicolas did that in v1/v2, but I wondered w.r.t. slow backend
or spotty and/or high-latency connection making this odd to use.
But I just found that my reply was not CC'ing the list, so I just
re-send it for the record:
https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/
That said, given that you suggested the other way around for UX, it
might be indeed better to keep it that way, or at least actually try
how that way works with a slow backend (developer tools can simulate
slow network (the chromium based ones are a bit more powerful in that
regard IIRC), or alternatively use a traffic control (tc) netem qdisc
that adds latency [0] can be added on the PVE server.
Because as mentioned, it could be fine your previous way and the one
Shannon also would prefer, I rather wanted to avoid that we just take
our rather perfect fast and sub-milliseconds lab environments as sole
base for UX decisions.
[0]: https://manpages.debian.org/trixie/iproute2/tc-netem.8.en.html
e.g. something like: tc qdisc add dev eth0 root netem delay 3000ms
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu
2025-10-07 9:16 ` Thomas Lamprecht
@ 2025-10-07 10:35 ` Nicolas Frey
2025-10-07 10:49 ` Thomas Lamprecht
0 siblings, 1 reply; 9+ messages in thread
From: Nicolas Frey @ 2025-10-07 10:35 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox VE development discussion, Shannon Sterz
Cc: pve-devel
On 10/7/25 11:16 AM, Thomas Lamprecht wrote:
> Am 07.10.25 um 10:42 schrieb Shannon Sterz:
>> 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.
>
> FWIW, Nicolas did that in v1/v2, but I wondered w.r.t. slow backend
> or spotty and/or high-latency connection making this odd to use.
> But I just found that my reply was not CC'ing the list, so I just
> re-send it for the record:
> https://lore.proxmox.com/pve-devel/9563cc43-f8ad-4b33-b8ea-a22d22b3405e@proxmox.com/
>
> That said, given that you suggested the other way around for UX, it
> might be indeed better to keep it that way, or at least actually try
> how that way works with a slow backend (developer tools can simulate
> slow network (the chromium based ones are a bit more powerful in that
> regard IIRC), or alternatively use a traffic control (tc) netem qdisc
> that adds latency [0] can be added on the PVE server.
> Because as mentioned, it could be fine your previous way and the one
> Shannon also would prefer, I rather wanted to avoid that we just take
> our rather perfect fast and sub-milliseconds lab environments as sole
> base for UX decisions.
>
> [0]: https://manpages.debian.org/trixie/iproute2/tc-netem.8.en.html
> e.g. something like: tc qdisc add dev eth0 root netem delay 3000ms
I used some variations of added network throttle using chrome dev
tools and traffic control and noticed that the disabling of the button
is starting to get really noticable in the 1-1.5 second range (if you
don't have sniper level mouse accuracy). Anything below that is
negotiable, but if the request stalls even a bit above that range it
could become an annoyance (e.g. being just about too early and having
to click a second time or simply the long wait). Though similarly, you
still have to wait for the window to appear if you clicked it in the
new version, and there may also be an error if snapshots aren't
supported on the guest.
As a note, the original implementation (v1/v2) matches the behaviour
found in the Snapshots tab, which has the button disabled until
querying snapshot features is complete. This also shows that the
`current guest does not support taking new snapshots` until fully
loaded. IMO parity between them is essential, as to not have two
different approaches which might confuse users.
I cannot tell which approach is better here, but it might be a sign
that Shannon had also suggested the other way...
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [pve-devel] [PATCH pve-manager v3 1/1] ui: fix #6209: create snapshots and backups from context menu
2025-10-07 10:35 ` Nicolas Frey
@ 2025-10-07 10:49 ` Thomas Lamprecht
0 siblings, 0 replies; 9+ messages in thread
From: Thomas Lamprecht @ 2025-10-07 10:49 UTC (permalink / raw)
To: Nicolas Frey, Proxmox VE development discussion
Am 07.10.25 um 12:34 schrieb Nicolas Frey:
> I used some variations of added network throttle using chrome dev
> tools and traffic control and noticed that the disabling of the button
> is starting to get really noticable in the 1-1.5 second range (if you
> don't have sniper level mouse accuracy). Anything below that is
> negotiable, but if the request stalls even a bit above that range it
> could become an annoyance (e.g. being just about too early and having
> to click a second time or simply the long wait). Though similarly, you
> still have to wait for the window to appear if you clicked it in the
> new version, and there may also be an error if snapshots aren't
> supported on the guest.
Thanks for checking so closely and reporting back!
> As a note, the original implementation (v1/v2) matches the behaviour
> found in the Snapshots tab, which has the button disabled until
> querying snapshot features is complete. This also shows that the
> `current guest does not support taking new snapshots` until fully
> loaded. IMO parity between them is essential, as to not have two
> different approaches which might confuse users.
>
> I cannot tell which approach is better here, but it might be a sign
> that Shannon had also suggested the other way...
Yeah, that, and this being basically pre-existing behavior in the
Snapshots tab, like you mentioned, is probably enough weight to tip the
scale towards your original approach.
Thanks for trying this out though, sometimes some experimentation is
needed, and even though it's only a small UX thing, these things tend to
get copied and thus leak often quite fast in other parts of the UI or
even UIs for our other projects, so it can be worth to look more
closely once in a while here.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 9+ messages in thread