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

feat: top level error catch #156

Merged
merged 1 commit into from
Jun 28, 2022
Merged

feat: top level error catch #156

merged 1 commit into from
Jun 28, 2022

Conversation

AuHau
Copy link
Contributor

@AuHau AuHau commented Jun 28, 2022

This catches on top-level errors and displays them as top-up. This is especially in order to not silently fail.

@AuHau AuHau force-pushed the feat/top-level-error-catch branch from 4776fb6 to fe94777 Compare June 28, 2022 08:52
@vojtechsimetka
Copy link
Contributor

Is the idea to provide sentry feedback here?

@agazso
Copy link
Member

agazso commented Jun 28, 2022

I would not do this. As a user it is very annoying when you get a meaningless error and you cannot do anything about it. It would make more sense to integrate this with the bug report dialog.

@AuHau
Copy link
Contributor Author

AuHau commented Jun 28, 2022

I would not do this. As a user it is very annoying when you get a meaningless error and you cannot do anything about it. It would make more sense to integrate this with the bug report dialog.

Well, but it is also very annoying if you launch the app and nothing happens ;-) So what is more annoying? IMHO this should be displayed only in edge-cases like when the app crashes. We should otherwise catch other errors... Will that be the case? I don't know, but I would at least try it for the pre-release and see. Btw. this will be displayed only if the app crashes (as the error otherwise goes into the Electron framework which exits the app).

@AuHau
Copy link
Contributor Author

AuHau commented Jun 28, 2022

Is the idea to provide sentry feedback here?

No, that Feedback form is not present in Electron "world". We would have to somehow open Dashboard or something and display it there, which IMHO is not good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants