-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix: Improve error handling for Glances widgets #3657
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a real look at this soon but my first thought is that the logic to respect errors should just be moved inside Error, instead of all the other changes for removing it. and in info and gpu where you check for the error property presumably that should become the error for the Error component
Hi, I don't necessarily disagree, the only reason I wrote the code this way is because the Error component for the glances widget essentially just returns the div with the localised translation:
So it was just to tidy the imports and dependencies up a bit. By the way, I'm on UK timezone so if I don't reply quickly I'm probably asleep! |
Sure, but separating them logically makes sense and that's also how the rest of the app does it. Also would significantly reduce the diff here. Let us know if youre OK making the change, I can also take care of it if thats better. |
Sure, let me make the changes and commit. |
I've now reintroduced the error component and import it within the container component. Following the implementation style in e.g. the Home Assistant widget
|
That wasnt exactly what I had in mind. Please take a look at the changes here. (also FYI https://eslint.org/docs/latest/rules/no-prototype-builtins ) and let me know if you have any thoughts or concerns |
Thanks for this, and thanks for the link that's good to know. As for the changes, it seems like there isn't a consistent implementation of the error handling (e.g. the Home Assistant ref I provided was similar to my own implementation but different to the one you have provided in the changes.) - either way the result will be the same and looks good to me. |
Point taken, I've gone back to a bit of a mix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks again
Proposed change
I have the Glances widgets deployed in my Homepage instance and noticed that when a host is unreachable the widgets do not handle the errors as the other widgets do. It results in an ugly error message which doesn't respect either the global or service-level
hideErrors
. Here is an example from my deployment:I have rewritten parts of the components such that the
Error
component is no longer needed, as it is now implemented within theContainer
component. The service and error attributes are now passed into theContainer
component, such that the service-levelhideErrors
flag can be checked. Additionally, the global settings are now imported into theContainer
component so the globalhideErrors
flag can be checked. Within each component, if the service data containers an "error" key, the widget will now display an error message in line with other widgets. See the screenshot below:Additionally, see the below screenshot testing the service-level
hideErrors
flag set totrue
for one widget:Type of change
Checklist: