Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Come up with a proper way to display errors in frontend #41

Closed
johanstokking opened this issue Jan 29, 2019 · 5 comments · Fixed by #667
Closed

Come up with a proper way to display errors in frontend #41

johanstokking opened this issue Jan 29, 2019 · 5 comments · Fixed by #667
Assignees
@kschiffer
Labels
c/console This is related to the Console in progress We're working on it needs/discussion We need to discuss this umbrella This issue needs actionable issues referenced
Milestone
May 2019

Comments

@johanstokking
Copy link
Member

johanstokking commented Jan 29, 2019 •
edited by kschiffer
Loading

Summary:
At the moment there is no real error handling on the frontend. We should make use of React's Error Boundaries here to scope error messages to their concerning components, as long as this is possible.
Additionally, we need a proper error page that will be shown when fatal errors occur that prevents the app from loading. This error page should match the static ones that are rendered by the backend.

Why do we need this?
Handling errors gracefully is essential for a UX that minimizes frustration. We should if possible also provide hints to the users as to how specific errors might be resolved.

What is already there? What do you see now?
There is some fragmented error handling already in place eg. for the login screen. Also we already have the necessary backend functionality to get error messages that have all the info that we need, as well as i18n capabilities. The <ErrorMessage />-component is specifically for this.

If the backend encounters an error in HTTP under /console or /oauth, it now renders HTML (if requested by browser in Accept header) with the error JSON in PAGE_DATA.error.

A request curl -H "Accept: text/html" -XPOST http://localhost:1885/oauth/api/auth/login results in the following (formatted for readability):

<!doctype html>
<html lang="en">

<head>
    <!-- ... -->
</head>

<body>
    <div id="app"></div>
    <script>
        window.APP_ROOT = "/oauth";
        window.ASSETS_ROOT = "http://localhost:1885/assets";
        window.APP_CONFIG = { "language": "en" };
        window.PAGE_DATA = {
            "error": {
                "code": 2,
                "message": "error:pkg/errors/web:http (HTTP error: code=403, message=invalid csrf token)",
                "details": [
                    {
                        "@type": "type.googleapis.com/ttn.lorawan.v3.ErrorDetails",
                        "namespace": "pkg/errors/web",
                        "name": "http",
                        "message_format": "HTTP error: {message}",
                        "attributes": {
                            "message": "code=403, message=invalid csrf token"
                        }
                    }
                ]
            }
        };
    </script>
    <script type="text/javascript" src="http://localhost:1885/assets/oauth.b4ebca19adbb1b50d911.js"></script>
</body>

</html>

Errors are structured in the same way as the errors you receive from the API, so with the details array, where you need to find the object with "@type": "type.googleapis.com/ttn.lorawan.v3.ErrorDetails" to get TTN error details.

We probably need a broader error message strategy. Four error message scenarios come to mind:

  • A full page error message -> Display full page errors for passed page error data #233
  • A component error overlay
  • An in-place error message (e.g. for invalid login credentials; after submit, not field validation), should probably be part of the component (this is more or less implemented via the <Notification /> component which can take error objects for display)
  • Error notifications/toasts ( -> this was implemented via Add toast notification #139 )

I propose the following designs:
The component error overlay I could envision like this:
image
In page example:
image
So it would be sort of an enhanced form of <Notification />, which spans the whole width of the errored component and displays a specific support message. This example shows an error of the device list, which I don't know whether we should provide an error boundary for. But I think it's good to show how the error overlay would work.

What is missing? What do you want to see?
A clear strategy for implementing this.

How do you propose to implement this?
By adding Error Boundaries via adding componentDidCatch() to all components in the webui that display fetched data and possibly adding a flag that will render the component as errored, which we should already add when we implement new components. This should also be put into storybook.

What can you do yourself and what do you need help with?
I can do this.

cc @bafonins

Original issue: https://github.com/TheThingsIndustries/lorawan-stack/issues/1308 by @kschiffer
@johanstokking johanstokking added c/console This is related to the Console l/open source labels Jan 29, 2019
@johanstokking johanstokking added this to the Backlog milestone Feb 6, 2019
@bafonins bafonins mentioned this issue Feb 12, 2019
Merged
@johanstokking johanstokking added the umbrella This issue needs actionable issues referenced label Mar 4, 2019
@johanstokking
Copy link
Member Author

@kschiffer what is the action here, how does it relate to toast and should this be broken down in smaller issues?

@johanstokking johanstokking modified the milestones: Backlog, Next Up Mar 4, 2019
@kschiffer kschiffer added the needs/discussion We need to discuss this label Mar 4, 2019
@kschiffer
Copy link
Contributor

I have updated the issue to include the toast PR. There are some elements of discussion still, but I will split this one up in smaller issues.

@johanstokking
Copy link
Member Author

OK, I'll leave it in Next Up for now, but please resolve discussion points shortly or make decisions here.

@kschiffer kschiffer mentioned this issue Mar 5, 2019
Closed
@kschiffer
Copy link
Contributor

Some follow-up thoughts about non-fatal errors:
I originally proposed error overlays similar to the notification component (see above). I think I was not thinking it through enough. One issue being what @bafonins already pointed out, that using error boundaries too lightly in our components could result in a page cluttered with error overlays, where a full view error would actually make more sense.

To me, it appears to make sense to split non fatal errors into categories:

  • Errors that prevent a whole view from rendering (or potentially resulting in a view that would make no sense to display)
  • Errors that are scoped to a specific container
    • This is not very relevant at the moment but might become so with event/data-streaming (Console support for event streams #28)
    • The use case I'm seeing there is for example connection issues to the data streaming endpoint, that prevents the streaming widget from being displayed in the overview pages, but it would not make sense to let the whole overview page crash then

So for the time being, I think we should focus on the first category. My proposal would be to add a little <ViewError /> component that will be rendered instead of the actual view and soberly explain to the user that something went wrong and guide to some resources that might help to resolve the issue (Try again later, file an issue, visit our forums / slack).
We could then add error boundaries to our parent views (console/views/application, console/views/gateways, etc.) that would take care of this and progressively add deeper boundaries later (in case we see that it makes sense).

Thoughts welcome.

@htdvisser htdvisser modified the milestones: Next Up, April 2019 Mar 26, 2019
@htdvisser htdvisser modified the milestones: April 2019, May 2019 Apr 23, 2019
This was referenced May 3, 2019
Closed
Merged
Closed
@htdvisser htdvisser added the in progress We're working on it label May 14, 2019
@kschiffer kschiffer mentioned this issue May 15, 2019
Merged
@kschiffer
Copy link
Contributor

Closed via #667

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kschiffer kschiffer

Labels
c/console This is related to the Console in progress We're working on it needs/discussion We need to discuss this umbrella This issue needs actionable issues referenced
Projects
None yet
Milestone
May 2019
Development

Successfully merging a pull request may close this issue.

Frontend Error Handling TheThingsNetwork/lorawan-stack
3 participants
@htdvisser @kschiffer @johanstokking