-
Notifications
You must be signed in to change notification settings - Fork 0
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
Improved offline support and API error indicators #157
Conversation
I'll review this PR later today. I skimmed the code diff on my phone a few days ago and did not notice any issues.
During this morning's squad meeting, I said I wanted the app to at least alert the user before changes they made while they were offline get written to the server (potentially overwriting changes made on the server since then, since the lock isn't being obtained for offline editing). I'll create a separate ticket about that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR description! Thanks for implementing all this. I did not come across anything that I consider to be a deal breaker. I left some suggestions about variable names, comment formatting, and other non-deal-breaker stuff.
Co-authored-by: eecavanna <134325062+eecavanna@users.noreply.github.com>
Co-authored-by: eecavanna <134325062+eecavanna@users.noreply.github.com>
Fixes #42
Summary
This is a bit of a pick-n-mix of change all in the neighborhood of API error messaging and offline support, since those two things are closely related. The expected functionality can be boiled down to:
Details
NetworkStatusProvider
provides a singleisOnline
boolean value which can be accessed through theuseNetworkStatus
hook. Under the hood this provider uses the@capacitor/network
plugin to get the initial online status and setup a lister to respond to changes in online status. React Query has its own OnlineManager which is how it knows when to pause or retry requests. However, I found there were certain cases where React Query wasn't as reliable as Capacitor at detecting connection changes. So theNetworkStatusProvider
also attempts to keep them in sync by manually setting React Query's network status.StoreProvider
has been updated to account for cases where the app starts up while offline. Specifically:nmdc-server
API client. Previously this happened only after a successful token exchange took place (which obviously can't happen if the app starts while offline).Unauthorized
response, the client will detect that, perform the token exchange at that point, and then retry the request. That logic was in place before, but the API client didn't have the refresh token it needed (see previous bullet point).IonProgressBar
have been removed individual components (e.g.StudyList
). Instead, one globalIonProgressBar
has been added toThemedToolbar
. It responds to any query or mutation managed by React Query.ThemedToolbar
has also been updated to show a banner when the user is offline.QueryErrorBanner
andMutationErrorBanner
, are responsible for displaying error messages when network requests fail. Both of those components are thin wrappers around the newErrorBanner
component, which is responsible for actually formatting the error message. This component does show (although not by default) the nitty-gritty technical details of the failed request. I have mixed feelings about this, but I have a feeling it might be useful at least during the beta testing period.Testing
nmdc-server
instance, though, because even in airplane mode the device knows it can make local (e.g.127.0.0.1
) requests. A web browser's dev tools can also simulate offline conditions but you cannot test starting the app in an offline state that way.nmdc-server
instance and have it unconditionally raise exceptions in specific request handler functions (i.e. innmdc_server/api.py
). Sometimes I would also intentionally add delays in order to verify the transitions between loading and error states. So for example: