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

Improved offline support and API error indicators #157

Merged
merged 24 commits into from
Aug 20, 2024

Conversation

pkalita-lbl
Copy link
Collaborator

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:

  • While offline:
    • Do not allow requests to create or delete a submission. This might be erring on the defensive side, but I think allowing those would lead to more weird edge cases than we're prepared to handle right now.
    • Do allow requests to update a submission. Such requests will be paused until the client is back online. Successive requests to update to the same submission should replace earlier requests so that one cumulative update request per submission goes out later.
    • Skip the requests to lock and unlock submissions. While offline we know that such a request can't be fulfilled, so in order to allow updates we simply have to presume that the lock is available.
  • While online
    • Let all requests go out across the network.
    • If any network request (query or mutation) is in progress, show a top-level loading indicator
    • If a network request fails (because the server is unreachable or the server responded with a non-2xx status), show an error indicator to the user
    • A failed query should not prevent cached data from being shown

Details

  • The new NetworkStatusProvider provides a single isOnline boolean value which can be accessed through the useNetworkStatus 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 the NetworkStatusProvider also attempts to keep them in sync by manually setting React Query's network status.
  • The initialization of StoreProvider has been updated to account for cases where the app starts up while offline. Specifically:
    • When a user logs in, store all of their user details in persistent storage (in addition to their refresh token, which was all that was previously stored)
    • When the app starts up, if user details are found in persistent storage use them to set the logged-in user state.
    • When the app starts up, if a refresh token is found in persistent storage immediately provide it to the 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).
    • When the app starts up and we're online, attempt the token exchange and fetch updated user details from the server. If the app starts up while offline, this token exchange won't happen. Instead, when the client does go online, the first network request will go out and fail with an 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).
  • Instances of IonProgressBar have been removed individual components (e.g. StudyList). Instead, one global IonProgressBar has been added to ThemedToolbar. 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.
  • Two new components, QueryErrorBanner and MutationErrorBanner, are responsible for displaying error messages when network requests fail. Both of those components are thin wrappers around the new ErrorBanner 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.
image image
  • I added a pull-to-refresh capability to the study list and individual study pages. Even though React Query is pretty smart about refetching at the right time, I think it's just comforting to be able to refresh on demand.
  • Lots of little logic changes around when certain requests go out or certain operations are allowed based on network status.

Testing

  • Offline support is probably best tested using an Android emulator. Airplane mode on the device can be used to force the app into an offline state. I would recommend pointing to the dev 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.
  • To test the API error messages (while online) I used my local nmdc-server instance and have it unconditionally raise exceptions in specific request handler functions (i.e. in nmdc_server/api.py). Sometimes I would also intentionally add delays in order to verify the transitions between loading and error states. So for example:
@router.get(
    "/metadata_submission",
    tags=["metadata_submission"],
    responses=login_required_responses,
    response_model=query.MetadataSubmissionResponse,
)
async def list_submissions(
    db: Session = Depends(get_db),
    user: models.User = Depends(get_current_user),
    pagination: Pagination = Depends(),
):
    query = crud.get_submissions_for_user(db, user)
    time.sleep(2)
    raise HTTPException(status_code=410, detail="whoops")
    return pagination.response(query)

@eecavanna
Copy link
Collaborator

eecavanna commented Aug 19, 2024

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.

Skip the requests to lock and unlock submissions. While offline we know that such a request can't be fulfilled, so in order to allow updates we simply have to presume that the lock is available.

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.

Copy link
Collaborator

@eecavanna eecavanna left a 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.

pkalita-lbl and others added 3 commits August 20, 2024 10:04
Co-authored-by: eecavanna <134325062+eecavanna@users.noreply.github.com>
Co-authored-by: eecavanna <134325062+eecavanna@users.noreply.github.com>
@pkalita-lbl pkalita-lbl merged commit 13b7657 into main Aug 20, 2024
1 check passed
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.

App UI does not show error when failing to access API despite having Internet access
2 participants