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
Handle configuration saving failures #152
Comments
Had a look into this and did a little testing by forcing errors in the code. If for any reason loading was working but saving wasn't, then the user would actually get the unhandled error page as they do for loading failure. This is because loading the configuration dispatches an event, which in-turn triggers a save. However, if this initial save works and only starts failing at some future point then the next save depends on user actions. Errors thrown inside event handlers are not caught by the React error boundary , so what happens depends on the action taken by the user. For example adding a success message results in an error being displayed to the user. This is because adding a success message uses the Form code which correctly handles errors thrown when submitting. However, removing a success message has no such error handling, meaning the error is not shown to the user. Also it appears as the redux store "rolls back", so the in-memory store isn't updated either. So in this example, the remove button would just appear to be doing nothing, as no error would be shown and the success message would not get removed. So to fix this issue properly, we'd need to add error handling code and displayable error messages to every single action that dispatches actions to the store. Given how rare I suspect this would be, I'm not sure its worth it and am considering just closing this issue 🤔 Anyone have any thoughts? |
Failure to load the configuration is fatal and the only real option is to show the user an error screen. We should include the exception message in the hopes it's detailed enough for the user to fix.Failing to load is now covered by bug #309 (edit: which is now fixed)
Failure to save is less fatal as the in-memory store will still update correctly and Nevergreen will continue to work, it's just any changes would be lost if the page was refreshed.
We need to have an obvious error message so users could manually backup the configuration.Edit: Given this is unlikely to happen and if it does the user probably has bigger problems, this is low priority and the simplest fix is to just show the user the same uncaught exception page.
The text was updated successfully, but these errors were encountered: