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: handle errors better #697

Merged
merged 5 commits into from
Nov 24, 2018
Merged

feat: handle errors better #697

merged 5 commits into from
Nov 24, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 19, 2018

If an error occurs while Electron is getting ready, we just show up a dialog telling that. There isn't much we can do. On the other hand, if the error happens during the execution of IPFS Desktop itself, we will have this error message:

image

The first option clearly opens the logs directory. The second option opens an URL to an issue pre-filled with the details of the error, like this:

image

This was something we didn't have in the last version, but we need to improve a lot: error handling! This will catch any major errors that might not be caught in other parts of the app.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@ghost ghost assigned hacdias Nov 19, 2018
@ghost ghost added the in progress label Nov 19, 2018
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc ipfs/ipfs-webui#774 (Opt-in error reporting and anonymous usage reporting)

I really like like the "opt-in" for creating github issue with error pre-filled, extremely useful!
It is a last resort, but at least gives the next step to the user and shows we care about improving 👍

Small asks in comments inline below.

src/index.js Outdated
buttons: [
'Close',
'Open logs',
'Create a new issue'
Copy link
Member

@lidel lidel Nov 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The label could be more inviting and explicit on what will happen on click:

Suggested change
'Create a new issue'
'Help us fixing it by creating a new issue with error details'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only regards I have here is that it is the name of a button. We are lucky they're big on Windows, but I have no clue how they'll look around the other OSes.

Copy link
Member

@lidel lidel Nov 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I tested how it looks on Linux:
2018-11-20--16-25-51
Not the prettiest, but also not the end of the world:
2018-11-20--16-26-42
Perhaps something like Report this issue to (IPFS) developers is a good compromise?
2018-11-20--16-30-54

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@olizilla
Copy link
Member

A proposal for a more human friendly error message

error-modal

The idea here is to

  • Clearly state the situation "IPFS Desktop has shutdown..."
  • Show just the error message, to give them something.
  • Hide the stack trace behind the collapsed details section. It's not much use to the end user.
  • Focus more on next steps.
  • Remove "open the logs" It's useful for devs, but not so much for users.

Of note, the github report thing is cool, but if you are not a github user and signed in, you'll be hit with a log-in / sign-up prompt, so we may want to let the user know that requirement before sending them off to report the error...

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Nov 22, 2018

Now it's like this:

image

@lidel
Copy link
Member

lidel commented Nov 22, 2018

Raw Linux (without DE) looks fine too 👍
2018-11-22--18-22-46

**Specifications**

- **OS**: ${process.platform}
- **IPFS Desktop Version**: ${app.getVersion()}
Copy link
Member

@lidel lidel Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something of note, app.getVersion returns electron version instead of one from package.json field:

2018-11-22--18-33-09

It seems to be a known issue (electron/electron#7085) when running development version from a subdirectory (./src/) and should be fine in release build (which uses ./), but would be good to fix it in dev in case someone builds it by hand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidel building by hand, i.e., npm run build will give you the correct value. Only if you run it directly with npm start you'll be given Electron's version. We could though read package.json file directly. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidel I'm merging this now. We can take a look at that detail later.

@hacdias hacdias merged commit b71cc73 into master Nov 24, 2018
@ghost ghost removed the in progress label Nov 24, 2018
@hacdias hacdias deleted the show-errors branch November 24, 2018 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants