gethomepage / homepage

A highly customizable homepage (or startpage / application dashboard) with Docker and service API integrations.
https://gethomepage.dev/
GNU General Public License v3.0
17.17k stars 991 forks source link

Fix: Improve error handling for Glances widgets #3657

Closed mjsully closed 2 weeks ago

mjsully commented 2 weeks ago

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:

glances-widget-bug

I have rewritten parts of the components such that the Error component is no longer needed, as it is now implemented within the Container component. The service and error attributes are now passed into the Container component, such that the service-level hideErrors flag can be checked. Additionally, the global settings are now imported into the Container component so the global hideErrors 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:

glances-widget-fix-showErrors

Additionally, see the below screenshot testing the service-level hideErrors flag set to true for one widget:

glances-widget-fix-hideErrors

Type of change

Checklist:

mjsully commented 2 weeks ago

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:

return <div className="absolute bottom-2 left-2 z-20 text-red-400 text-xs opacity-75">{t("widget.api_error")}</div>;

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!

shamoon commented 2 weeks ago

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.

mjsully commented 2 weeks ago

Sure, let me make the changes and commit.

mjsully commented 2 weeks ago

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 https://github.com/gethomepage/homepage/blob/ec448d6c41ffa7c90c4a180a972eff309ebbcf3d/src/widgets/homeassistant/component.jsx#L10 the error handling is still done within the container component. Let me know if you want any other changes making.

shamoon commented 2 weeks ago

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

mjsully commented 2 weeks ago

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.

shamoon commented 2 weeks ago

Point taken, I've gone back to a bit of a mix