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.

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

Open
GentlemanHal opened this issue Dec 17, 2016 · 1 comment
Open

Handle configuration saving failures #152

GentlemanHal opened this issue Dec 17, 2016 · 1 comment
Labels
complicated Changes that are difficult or time consuming to implement improvement Changes that improve an existing feature low priority Changes the team are unlikely to work on any time soon, consider submitting a PR

Comments

@GentlemanHal
Copy link
Member

GentlemanHal commented Dec 17, 2016

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.

@GentlemanHal GentlemanHal added the improvement Changes that improve an existing feature label Dec 17, 2016
@GentlemanHal GentlemanHal added this to the v1.0.0 milestone Dec 17, 2016
@GentlemanHal GentlemanHal added the a11y Changes related to user experience or accessibility label Jan 11, 2017
@GentlemanHal GentlemanHal removed this from the v1.0.0 milestone Jun 5, 2017
@GentlemanHal GentlemanHal removed this from Backlog in v1.0.0 Jun 5, 2017
@GentlemanHal GentlemanHal added this to the v7.0.0 milestone Aug 18, 2019
@GentlemanHal GentlemanHal changed the title Handle configuration loading/saving failures Handle configuration saving failures Jun 7, 2020
@GentlemanHal GentlemanHal added low priority Changes the team are unlikely to work on any time soon, consider submitting a PR and removed a11y Changes related to user experience or accessibility labels Jun 7, 2020
@GentlemanHal GentlemanHal removed this from the Settings update milestone Nov 22, 2022
@GentlemanHal GentlemanHal added the complicated Changes that are difficult or time consuming to implement label Feb 19, 2023
@GentlemanHal
Copy link
Member Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complicated Changes that are difficult or time consuming to implement improvement Changes that improve an existing feature low priority Changes the team are unlikely to work on any time soon, consider submitting a PR
Projects
None yet
Development

No branches or pull requests

1 participant