[pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Shannon Sterz]

otherwise users will get a `b.store is null` error in the console and
a loading spinner is shown for a while.

the issue in question seems to stem from the event handler that gets
attached when the "Prune & GC Jobs" tab is opened for a specific
datastore. however, that event handler should *not* be attached for
the "Datastore" -> "Prune & GC Jobs" panel. it seems that the event
handler does still get attached, and will fire in the "Datastore"
view if it hasn't fired while opened in a specific datastore
(it should only trigger a single time).

that scenario seems to occur when a different tab was previously
selected in a specific datastore and navigation is triggered via the
side bar from the "Datastore" -> "Prune GC Jobs" to a specific
datastore. that leads to the "Prune & GC Jobs" view for that specific
datastore being opened very briefly in which the event handler gets
attached, navigation then automatically moves to the previously
selected tab. this will stop the store from updating ensuring that
the event is never triggered. when we then move to
the "Datastore" -> "Prune & GC Jobs" tab again the event handler will
be triggered but the store of the view is null leading to the error.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 www/config/GCView.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/www/config/GCView.js b/www/config/GCView.js
index a6e79fb3..51ce1cb6 100644
--- a/www/config/GCView.js
+++ b/www/config/GCView.js
@@ -33,7 +33,11 @@ Ext.define('PBS.config.GCJobView', {
 		// after the store is loaded, select the row to enable the Edit,.. buttons
 		store.rstore.proxy.on({
 		    'afterload': {
-			fn: () => view.getSelectionModel().select(0),
+			fn: () => {
+			    if (view.store) {
+				view.getSelectionModel().select(0);
+			    }
+			},
 			single: true,
 		    },
 		});
-- 
2.39.5



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

Re: [pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Fiona Ebner]

Am 28.11.24 um 14:49 schrieb Shannon Sterz:
> otherwise users will get a `b.store is null` error in the console and
> a loading spinner is shown for a while.
> 
> the issue in question seems to stem from the event handler that gets
> attached when the "Prune & GC Jobs" tab is opened for a specific
> datastore. however, that event handler should *not* be attached for
> the "Datastore" -> "Prune & GC Jobs" panel. it seems that the event
> handler does still get attached, and will fire in the "Datastore"
> view if it hasn't fired while opened in a specific datastore
> (it should only trigger a single time).
> 
> that scenario seems to occur when a different tab was previously
> selected in a specific datastore and navigation is triggered via the
> side bar from the "Datastore" -> "Prune GC Jobs" to a specific
> datastore. that leads to the "Prune & GC Jobs" view for that specific
> datastore being opened very briefly in which the event handler gets
> attached, navigation then automatically moves to the previously
> selected tab. this will stop the store from updating ensuring that
> the event is never triggered. when we then move to
> the "Datastore" -> "Prune & GC Jobs" tab again the event handler will
> be triggered but the store of the view is null leading to the error.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  www/config/GCView.js | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/www/config/GCView.js b/www/config/GCView.js
> index a6e79fb3..51ce1cb6 100644
> --- a/www/config/GCView.js
> +++ b/www/config/GCView.js
> @@ -33,7 +33,11 @@ Ext.define('PBS.config.GCJobView', {
>  		// after the store is loaded, select the row to enable the Edit,.. buttons
>  		store.rstore.proxy.on({
>  		    'afterload': {
> -			fn: () => view.getSelectionModel().select(0),
> +			fn: () => {
> +			    if (view.store) {
> +				view.getSelectionModel().select(0);

In my testing, view.store is set if I was previously at a datastore's
"Prune & GC Jobs" but not if I was on a different tab from a datastore.
In both cases, the row does not seem to be selected and the "Edit" and
"Run Now" buttons are still grayed out. So your patch is certainly an
improvement, because there is no error and loading :) But it still
doesn't seem to do what was intended according to the code comment.

> +			    }
> +			},
>  			single: true,
>  		    },
>  		});



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

Re: [pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Fiona Ebner]

Am 28.11.24 um 15:15 schrieb Fiona Ebner:
> Am 28.11.24 um 14:49 schrieb Shannon Sterz:
>> otherwise users will get a `b.store is null` error in the console and
>> a loading spinner is shown for a while.
>>
>> the issue in question seems to stem from the event handler that gets
>> attached when the "Prune & GC Jobs" tab is opened for a specific
>> datastore. however, that event handler should *not* be attached for
>> the "Datastore" -> "Prune & GC Jobs" panel. it seems that the event
>> handler does still get attached, and will fire in the "Datastore"
>> view if it hasn't fired while opened in a specific datastore
>> (it should only trigger a single time).
>>
>> that scenario seems to occur when a different tab was previously
>> selected in a specific datastore and navigation is triggered via the
>> side bar from the "Datastore" -> "Prune GC Jobs" to a specific
>> datastore. that leads to the "Prune & GC Jobs" view for that specific
>> datastore being opened very briefly in which the event handler gets
>> attached, navigation then automatically moves to the previously
>> selected tab. this will stop the store from updating ensuring that
>> the event is never triggered. when we then move to
>> the "Datastore" -> "Prune & GC Jobs" tab again the event handler will
>> be triggered but the store of the view is null leading to the error.
>>
>> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
>> ---
>>  www/config/GCView.js | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/www/config/GCView.js b/www/config/GCView.js
>> index a6e79fb3..51ce1cb6 100644
>> --- a/www/config/GCView.js
>> +++ b/www/config/GCView.js
>> @@ -33,7 +33,11 @@ Ext.define('PBS.config.GCJobView', {
>>  		// after the store is loaded, select the row to enable the Edit,.. buttons
>>  		store.rstore.proxy.on({
>>  		    'afterload': {
>> -			fn: () => view.getSelectionModel().select(0),
>> +			fn: () => {
>> +			    if (view.store) {
>> +				view.getSelectionModel().select(0);
> 
> In my testing, view.store is set if I was previously at a datastore's
> "Prune & GC Jobs" but not if I was on a different tab from a datastore.
> In both cases, the row does not seem to be selected and the "Edit" and
> "Run Now" buttons are still grayed out. So your patch is certainly an
> improvement, because there is no error and loading :) But it still
> doesn't seem to do what was intended according to the code comment.
> 

From a quick off-list discussion, Shannon pointed out that this code is
intended for the "Prune & GC Jobs" tab for a single datastore, not the
overview, where it still works after the change :)

So consider this:

Tested-by: Fiona Ebner <f.ebner@proxmox.com>


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

Re: [pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Shannon Sterz]

On Thu Nov 28, 2024 at 3:15 PM CET, Fiona Ebner wrote:
> Am 28.11.24 um 14:49 schrieb Shannon Sterz:
> > otherwise users will get a `b.store is null` error in the console and
> > a loading spinner is shown for a while.
> >
> > the issue in question seems to stem from the event handler that gets
> > attached when the "Prune & GC Jobs" tab is opened for a specific
> > datastore. however, that event handler should *not* be attached for
> > the "Datastore" -> "Prune & GC Jobs" panel. it seems that the event
> > handler does still get attached, and will fire in the "Datastore"
> > view if it hasn't fired while opened in a specific datastore
> > (it should only trigger a single time).
> >
> > that scenario seems to occur when a different tab was previously
> > selected in a specific datastore and navigation is triggered via the
> > side bar from the "Datastore" -> "Prune GC Jobs" to a specific
> > datastore. that leads to the "Prune & GC Jobs" view for that specific
> > datastore being opened very briefly in which the event handler gets
> > attached, navigation then automatically moves to the previously
> > selected tab. this will stop the store from updating ensuring that
> > the event is never triggered. when we then move to
> > the "Datastore" -> "Prune & GC Jobs" tab again the event handler will
> > be triggered but the store of the view is null leading to the error.
> >
> > Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> > ---
> >  www/config/GCView.js | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/www/config/GCView.js b/www/config/GCView.js
> > index a6e79fb3..51ce1cb6 100644
> > --- a/www/config/GCView.js
> > +++ b/www/config/GCView.js
> > @@ -33,7 +33,11 @@ Ext.define('PBS.config.GCJobView', {
> >  		// after the store is loaded, select the row to enable the Edit,.. buttons
> >  		store.rstore.proxy.on({
> >  		    'afterload': {
> > -			fn: () => view.getSelectionModel().select(0),
> > +			fn: () => {
> > +			    if (view.store) {
> > +				view.getSelectionModel().select(0);
>

we've already talked about this off-list, but i wanted to document that
a bit better, so here goes nothing:

> In my testing, view.store is set if I was previously at a datastore's
> "Prune & GC Jobs" but not if I was on a different tab from a datastore.

yeah this is super confusing and requires setting the break points just
right to get to. datastore is set, if it was set by navigating to a
specific datastore's "Prune & GC Jobs" tab first as you pointed out and
isn't reset when you are in the top level "Datastore" panel.

in testing i tried to explicitly reset it to `undefined` in the
"Datastore" -> "Prune & GC Jobs" panel, but that would only avoid this
bug in certain situations. it was still possible to trigger it with the
navigation pattern described above.

> In both cases, the row does not seem to be selected and the "Edit" and
> "Run Now" buttons are still grayed out. So your patch is certainly an
> improvement, because there is no error and loading :) But it still
> doesn't seem to do what was intended according to the code comment.

it does select the row logically only in the specific datastore's "Prune
& GC Jobs" panel, as there should only ever be one GC job there. this
enables the "Edit" and "Run now" buttons without the user having to
select the single line in the table. (note that the highlighting is
removed, so visually nothing *looks* selected)

however, for the overview of all GC Jobs in the "Datastore" -> "Prune &
GC Jobs" panel, nothing should be auto-selected. hence, the `if
(view.datastore)` check before the event handler is attached. as
described in my commit message, it is still possible under certain
circumstance to have the event handler be attached and be triggered
here.

> > +			    }
> > +			},
> >  			single: true,
> >  		    },
> >  		});



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

Re: [pbs-devel] [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Gabriel Goller]

Thanks for fixing this!

Consider:
Tested-by: Gabriel Goller <g.goller@proxmox.com>



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

[pbs-devel] applied: [PATCH proxmox-backup] ui: check that store is set before trying to select in GCJobView

[Thomas Lamprecht]

Am 28.11.24 um 14:49 schrieb Shannon Sterz:
> otherwise users will get a `b.store is null` error in the console and
> a loading spinner is shown for a while.
> 
> the issue in question seems to stem from the event handler that gets
> attached when the "Prune & GC Jobs" tab is opened for a specific
> datastore. however, that event handler should *not* be attached for
> the "Datastore" -> "Prune & GC Jobs" panel. it seems that the event
> handler does still get attached, and will fire in the "Datastore"
> view if it hasn't fired while opened in a specific datastore
> (it should only trigger a single time).
> 
> that scenario seems to occur when a different tab was previously
> selected in a specific datastore and navigation is triggered via the
> side bar from the "Datastore" -> "Prune GC Jobs" to a specific
> datastore. that leads to the "Prune & GC Jobs" view for that specific
> datastore being opened very briefly in which the event handler gets
> attached, navigation then automatically moves to the previously
> selected tab. this will stop the store from updating ensuring that
> the event is never triggered. when we then move to
> the "Datastore" -> "Prune & GC Jobs" tab again the event handler will
> be triggered but the store of the view is null leading to the error.
> 
> Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
> ---
>  www/config/GCView.js | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
>

applied, with Fiona's and Gabriel's T-b, thanks!


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