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

Improve error handling when offline #2128

Merged
merged 35 commits into from May 14, 2019

Conversation

@smashwilson
Copy link
Member

commented May 7, 2019

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

When the network is unavailable, the GitHub package behaves poorly:

  • When starting or reloading Atom, the "log in to GitHub" view is shown from each Relay-backed view, which, of course, doesn't work.
  • If the network becomes unavailable after Atom is already open, the next attempted network operation brings down the package hard with an uncaught exception.
  • Relay actions that users explicitly initiate (like clicking on a refresh button, or triggering a mutation) uniformly create Atom error notifications, with customized messaging for the offline case.

I'm introducing:

  • A special <QueryErrorView> case that recognizes network errors and offers to open the dev tools for more investigation. (Unfortunately, the errors thrown by fetch are TypeError instances with little information and a message of "fetch failed.")
  • Create an alternate blank-slate case to show in place of the <GithubLoginView> when the network is unavailable.
  • Ensure that further changes in network state are detected when offline so we can re-render again.
  • Separate messaging for "offline" from "non-200, non-401 statuses"
  • Handle the issueish tiles in the GitHub tab
  • Preserve existing data and notify instead if a refetch fails, so you don't lose cached data.

Screenshots

When a user attempts to open a new item that uses Relay data (an issueish detail item, the reviews tab) while the network is offline, including items created during window state deserialization (like a reload):

offline message

When a user attempts to click a refresh button or perform a mutation while offline:

Screen Shot 2019-05-09 at 3 22 52 PM

When the network dies in between the git fetch completing and the Relay refetch triggering somehow:

offline-tile

Alternate Designs

N/A

Benefits

Users on spotty wi-fi or airplanes can have a gracefully degraded Atom experience and informative error states.

Possible Drawbacks

The errors thrown by the fetch API are (deliberately, I believe) very vague. It's possible that we could report that a user is offline when they have misconfigured DNS or proxy settings, for example. The underlying OS network error is shown in the developer tools, but we have no way to read it.

Applicable Issues

Fixes #1844.

Metrics

N/A

Tests

I did manual testing both by toggling network connectivity in the dev tools and by actually disabling and enabling wi-fi.

Interestingly, the 'online' event that I'm using to auto-retry from an OfflineView doesn't seem to fire the first time you uncheck the "Offline" button in the dev tools, but it does every subsequent time. The event fired when I expected it to when the wi-fi was actually restored so I didn't investigate further.

Documentation

N/A

Release Notes

  • The GitHub package reports clearer errors when no network connection is available.

User Experience Research (Optional)

N/A

@codecov

This comment has been minimized.

Copy link

commented May 7, 2019

Codecov Report

Merging #2128 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2128      +/-   ##
==========================================
+ Coverage   92.53%   92.68%   +0.14%     
==========================================
  Files         212      213       +1     
  Lines       12171    12188      +17     
  Branches     1779     1788       +9     
==========================================
+ Hits        11263    11296      +33     
+ Misses        908      892      -16
Impacted Files Coverage Δ
lib/views/reviews-view.js 87.8% <ø> (ø) ⬆️
lib/items/reviews-item.js 100% <ø> (ø) ⬆️
lib/controllers/issueish-detail-controller.js 100% <ø> (ø) ⬆️
lib/items/issueish-detail-item.js 98.38% <ø> (ø) ⬆️
lib/containers/reviews-container.js 100% <100%> (+4.76%) ⬆️
lib/controllers/emoji-reactions-controller.js 100% <100%> (ø) ⬆️
lib/controllers/reviews-controller.js 100% <100%> (ø) ⬆️
lib/models/github-login-model.js 89.65% <100%> (-0.18%) ⬇️
lib/containers/remote-container.js 100% <100%> (ø) ⬆️
lib/views/error-view.js 100% <100%> (+11.11%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update edbb26d...8b47553. Read the comment docs.

@smashwilson smashwilson marked this pull request as ready for review May 9, 2019

@smashwilson smashwilson requested a review from atom/github-package May 9, 2019

@kuychaco kuychaco referenced this pull request May 10, 2019
9 of 9 tasks complete
@vanessayuenn
Copy link
Contributor

left a comment

Thanks for tackling this! The new offline view looks good and the message is very clear 👍 . I have got a couple of questions; they're mainly for my own understanding and are not blockers.

test/views/query-error-tile.test.js Show resolved Hide resolved
lib/views/query-error-tile.js Show resolved Hide resolved
lib/views/error-view.js Outdated Show resolved Hide resolved
lib/views/error-view.js Outdated Show resolved Hide resolved
lib/views/error-view.js Outdated Show resolved Hide resolved
lib/views/query-error-tile.js Show resolved Hide resolved

smashwilson added some commits May 7, 2019

Flag network errors
Also pre-populate the `rawStack` property, so Atom doesn't try to do it 
after the Error is marked immutable and crashes hard.
Revert "Missed a drilling spot"
This reverts commit 0b3959a.

smashwilson added some commits May 9, 2019

@smashwilson smashwilson force-pushed the aw/die-better-when-offline branch from 8f1b5f5 to d29884d May 13, 2019

smashwilson added some commits May 14, 2019

@smashwilson smashwilson merged commit 14079a6 into master May 14, 2019

14 of 15 checks passed

Consider pull request for release board Consider pull request for release board
Details
atom.github Build #20190514.29 succeeded
Details
atom.github (Lint) Lint succeeded
Details
atom.github (Linux beta) Linux beta succeeded
Details
atom.github (Linux dev) Linux dev succeeded
Details
atom.github (Linux stable) Linux stable succeeded
Details
atom.github (MacOS beta) MacOS beta succeeded
Details
atom.github (MacOS dev) MacOS dev succeeded
Details
atom.github (MacOS stable) MacOS stable succeeded
Details
atom.github (Snapshot) Snapshot succeeded
Details
atom.github (Windows beta) Windows beta succeeded
Details
atom.github (Windows dev) Windows dev succeeded
Details
atom.github (Windows stable) Windows stable succeeded
Details
codecov/patch 100% of diff hit (target 92.53%)
Details
codecov/project 92.68% (+0.14%) compared to edbb26d
Details

@smashwilson smashwilson deleted the aw/die-better-when-offline branch May 14, 2019

@smashwilson smashwilson added this to In progress in Release : 9 May 2019 - 5 June 2019 : v0.30.0 via automation Jul 18, 2019

@smashwilson smashwilson referenced this pull request Jul 18, 2019
4 of 4 tasks complete

@smashwilson smashwilson moved this from In progress to Merged in Release : 9 May 2019 - 5 June 2019 : v0.30.0 Jul 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.