digidem / comapeo-mobile

The next version of Mapeo mobile
GNU General Public License v3.0
5 stars 1 forks source link

Viewing stale data after syncing #585

Open achou11 opened 2 months ago

achou11 commented 2 months ago

We currently don't invalidate any query caches when data is updated (in the background) via syncing. This leads to cases where certain parts of UI are outdated because they are displaying stale data. An example of this happening is navigating to screens that are part of the same stack navigation (e.g. the map) as the sync screen after syncing

Short-term suggestion is to invalidate all relevant queries when leaving the sync screen.

gmaclennan commented 2 months ago

Thinking about this more, you could invalidate queries on completing or stopping sync (outside the context of screens that have mounted) for a more long-term solution.

gmaclennan commented 2 months ago

I investigated this a bit more today.

  1. We are currently invalidating queries when we unmount the sync screen. I had missed that before.
  2. We should invalidate all queries after sync (we only need to invalidate for the current projectId, but that is not possible and not necessary right now). Otherwise we may get cases when presets don't show up as expected (unlikely with the current navigation, because screens with categories re-mount every time)
  3. Switching tabs and navigating to other pages and returning to the map screen does not remount the map screen. Similarly the observation list tab does not remount during navigation. We should not rely on that for avoiding stale data anyway.
  4. Similarly navigating between tabs or to other menus does not trigger refetches. Only navigating to the sync screen and then leaving it causes a refetch because of the invalidation in the beforeRemove event
  5. The current sync query invalidation is probably causing a race condition that explains the behaviour observed in QA. Stopping sync is an asyncronous operation. What I think is possibly happening:

    1. Unmount of sync screen calls project.$sync.stop()
    2. Observation queries are invalidated via queryClient.invalidateQueries({queryKey: [OBSERVATION_KEY]});
    3. This triggers tanstack query to refetch observations.
    4. This refetch in some cases happens before indexing starts from sync.
    5. Because indexing has not yet started, the API doesn't wait for indexing, and returns "stale" data.

The conditions for this are going to be device-dependent. I would expect indexing to start quite soon after a device receives data from sync, but it's conceivable that there is a delay maybe by sync occupying the event loop. It would depend on timing for when the user exits the sync screen after sync and how much data is syncing and how fast their device is.

I think a solution to this is to invalidate the queries once sync is in the stopped state. Although even more robust solution would also be to invalidate queries whenever syncing is completed (in case we decide in the future to not stop sync when we leave the screen). This listener ideally sits outside any component (e.g. in the sync store), but is ok to have in the sync screen for now.

Separately I think it would be useful to add a lot more debugging info to Sentry so we can determine the order of events for stuff like this.