-
-
Notifications
You must be signed in to change notification settings - Fork 828
Add error boundaries to catch rendering errors #3512
Conversation
This adds a basic error boundary around the entire app to catch errors during rendering and present the user with the options on how to proceed. This is not implemented as a modal so that it could be used selectively in portions of the app as well, such as just the `RoomView`. Fixes element-hq/element-web#11009
This adds a more specific boundary around the `RoomView` for room-specific errors and is an example how we could use add boundaries around just a portion of the app.
@nadonomy, I expect that you will also have opinions on this. 😄 Here's some context on what this is: For bugs in the React components that make up the Riot UI, the new version of React we're now using allows us to catch such errors and present some kind of error UI to the user. When such an error happens, it's possible that the UI could be in an invalid state, so I have so far followed the React 16 best practice of visibly failing to user and offering a path to report the problem. This ensure the user doesn't fall into a trap of e.g. sending a message to the wrong room in case some UI bug failed to update the current room or something similar. |
I'll be away Thursday - Friday, so feel free to take over / merge etc. this PR if you want to land this before I am back on Monday. |
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.
Code-wise looks good,
LGTM pending Design review
Yes! I have major concerns re being so explicit pointing end users to GitHub directly— however we already do that in Settings etc so won't block this PR on those concerns, instead tracking here: element-hq/element-web#11038 It'd be great to lighten up the design, as right now it's super text heavy which will lead users to not actually read or action it. I'm thinking we could:
Is that last point undesirable? Do we want to avoid spawning modals from here in case they fail to render, or if the user is experiencing this error in a modal already? In this PR so far, what happens when a user clicks on [Submit debug logs]? Do we display an in-line spinner/confirmation message like when submitting debug logs via settings? |
It looks like it spawns the bug report dialog (the one with a text field), which should already have the applicable warning text about what logs contain (I think/hope). Also sounds like it is safe to spawn dialogs given we do it for bug reports :D |
Got it. And, would it be advisable for users to hit refresh on their browser? |
As per standup convo with @lampholder, going to merge this because a rough & ready error screen is better than a blank white screen which is what our users will be getting atm. |
@nadonomy it'd be better if people sent us logs and then refreshed, yes. |
This adds a basic error boundary around the entire app to catch errors during
rendering and present the user with the options on how to proceed. This is not
implemented as a modal so that it could be used selectively in portions of the
app as well, such as just the
RoomView
.This also adds a more specific boundary around just the
RoomView
for in-room errors. We could consider similar such boundaries around other main content areas in the future.These rendering errors are latent bugs in the app that were previously being hidden by React 15's behaviour of just logging to the console and attempting soldier on anyway. For code hygiene, it does seem best to follow React 16's best practice of stopping (a portion of) the app for such an error so we actually fix it, but we may decide the cost to users is too high. We could change this boundary component then to only log the error to the console and still render children anyway, which effectively be similar to React 15 behaviour.
Fixes element-hq/element-web#11009