zooniverse / front-end-monorepo

A rebuild of the front-end for zooniverse.org
https://www.zooniverse.org
Apache License 2.0
104 stars 30 forks source link

`mst-persist` changes expected "Reload Page" behaviour in Classifier (e.g. workflow changes not showing immediately) #2668

Closed shaunanoordin closed 2 years ago

shaunanoordin commented 2 years ago

Functionality Bug/Quirk(?)

Package: lib-classifier

As a result of mst-persist in #2627, reloading the Classifier does not fetch any recent changes to the Workflows.

Example:

Explanation: This is, of course, because the WF data is stored in sessionStorage (which isn't wiped even by a hard reload), and the Classifier has no idea there's a newer version of the WF data on the server. (See FEM PR #2627)

Solution/workaround is simple: open a new tab, instead of just reloading. (Or clear your sessionStorage.)

That said, I can see how this can be very confusing, since the persist-in-localStorage is basically overriding the expected behaviour of the Reload Page action.

I don't believe this will significantly affect volunteers (unless we have volunteers who never close their tabs) but it's worth keeping some eyes on this. Project owners might be a bit more affected, as Delilah pointed out. (i.e. they'd have one tab open to edit the project, and another tab with the project which they'll keep refreshing, only to see no changes.)

Status

Medium priority. We have a simple workaround, but that's not yet communicated with the volunteers.

Work with mst-persist is ongoing; tagging @eatyourgreens on the matter.

shaunanoordin commented 2 years ago

Additional notes

eatyourgreens commented 2 years ago

This isn't an mst-persist bug as such, but rather a bug in the data fetching logic, in that we don't fetch fresh data for a resource store if that store's loading state is already set eg. here: https://github.com/zooniverse/front-end-monorepo/blob/a061118bed8ddd8dfa8326c6ffb70b8c85f03ae4/packages/lib-classifier/src/store/TutorialStore.js#L120-L124

eatyourgreens commented 2 years ago

Perhaps here too, where we don't load a resource if there's already one with the same ID in the map: https://github.com/zooniverse/front-end-monorepo/blob/a061118bed8ddd8dfa8326c6ffb70b8c85f03ae4/packages/lib-classifier/src/store/ResourceStore/ResourceStore.js#L66-L73

eatyourgreens commented 2 years ago

Would it be helpful to disable the storage cache, by default, in the dev classifier? That would be a fairly quick, and small, PR.

shaunanoordin commented 2 years ago

I have a few thoughts on improvements:

eatyourgreens commented 2 years ago

Maybe a Boolean prop on the Classifier component to enable/disable caching?

I was thinking a TTL on stored resources would be good too, via the Resource model.

eatyourgreens commented 2 years ago

Preview links, particularly for private workflows, should probably use preview mode in NextJS. https://nextjs.org/docs/advanced-features/preview-mode

shaunanoordin commented 2 years ago

Curses! ☠️ I can confirm that the first Tutorial in a session is incorrectly persisted when you switch Workflows. Documented in #2670

eatyourgreens commented 2 years ago

All good suggestions here. I’ll open a PR on Monday with fixes.

eatyourgreens commented 2 years ago

I think the strategy we want here is stale-while-revalidate.

https://swr.vercel.app/

SWR is the data fetching strategy used in the NextJS apps.

  1. Load stale, cached data and show that immediately.
  2. Fetch fresh data in the background and overwrite the existing resources when it arrives.

So, get rid of self.reset() when updating the store. Also, check what happens if you call store.resources.put(resource) with an existing resource. Does it overwrite with new data? If so, does that fire any reactions that are observing the stored resource?

eatyourgreens commented 2 years ago

Having said that, map.put(resource) isn't an ES6 Map method, and isn't mentioned in the MobX docs either.

https://mobx.js.org/api.html#observablemap

eatyourgreens commented 2 years ago

useSWR, with multiple arguments, looks useful for fetching the workflow when the classifier first loads. https://swr.vercel.app/docs/arguments