Classifier: Refactor and test useHydratedStore hook. Remove mst-persist

[eatyourgreens]

• consolidate useStore and useHydratedStore, so that useHydratedStore replaces useStore to create the new store from an optional snapshot.
• move the classifier hooks into src/hooks, aliased to @hooks.
• remove mst-persist.

This continues #2793, consolidating the hooks that are used to create the classifier's MST store. The Classifier now calls useHydratedStore, which replaces useStore to create a new store.

const classifierStore = useHydratedStore(storeOptions, cachePanoptesData, `fem-classifier-${project.id}`)

If the cachePanoptesData flag is set, the new store is created from a saved snapshot, using RootStore.create(snapshot, storeOptions), rather than applySnapshot(store, snapshot). This means that lib-classifier uses the same data flow, and logic, as we use in app-project to build the MST store in the browser.

I've also removed mst-persist, but I've copied its onSnapshot handler into the useHydratedStore hook.
https://github.com/agilgur5/mst-persist/blob/ae86acceb931ff82a8081fefb9a82f639d6da43a/src/index.ts#L48-L49

Testing

Davy Notebooks has session storage enabled and can be used for testing single workflows in the project app:
https://local.zooniverse.org:3000/projects/humphrydavy/davy-notebooks-project/classify?env=production&demo=true

To test this out in the dev classifier, with all the indexing features enabled, change cachePanoptesData from false to true in dev/App.js and load in the RBGE project with a subject set and subject selected:
https://local.zooniverse.org:8080/?project=emhaston/the-rbge-herbarium-exploring-gesneriaceae-the-african-violet-family&workflow=19381&subjectSet=99948&subject=70656987&env=production

You can also try the RGBE project in a production build of the project app at:
https://local.zooniverse.org:3000/projects/emhaston/the-rbge-herbarium-exploring-gesneriaceae-the-african-violet-family/classify/workflow/19381/subject-set/99948/subject/70656991

Package:
lib-classifier

Review Checklist

General

• Are the tests passing locally and on Travis?
• Is the documentation up to date?

Components

• Has a storybook story been created or updated?
• Is the component accessible?
  • Can it be used with a screen reader? BBC guide to testing with VoiceOver
  • Can it be used from the keyboard? WebAIM guide to keyboard testing
  • Is it passing accessibility checks in the storybook?

Apps

• Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
• Does it work on mobile?
• Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

• Is the changelog updated?
• Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

• Did the app deploy to https://frontend.preview.zooniverse.org/projects/:project-name/:owner or https://frontend.preview.zooniverse.org/about?
• Is the new feature working or bug now fixed?
  • Is there a Talk or blog post written to announce the new feature(s)?
• Is the design working across browsers (Firefox, Chrome, Edge, Safari) and mobile?
  • Is this approved by our designer?
• Is this ready for production deployment?

[shaunanoordin]

PR Review

Package: lib-classifier (tested on app-project)
Related to: (mostly) Engaging Crowds projects (for now)

This PR is a refactor for the Classifier store's behaviour, where - if the project is explicitly set to cache data from Panoptes - the code first pulls a snapshot from Session Storage to populate the MST Store, and then tries to get newer data from Panoptes.

• The mst-persist dependency is now removed.
• There should be no visible changes to the user for either normal-projects or projects-with-cache. A user should be able to 1. load workflows, 2. change workflows, and 3. (for projects-with-cache) go to the Talk page or etc and press the Back button to return to a Classify page that still works fine.

Code read looks mostly good (see my dev ramblings below if you really want), but testing will be a mild pain in the butt.

Dev Notes

Thoughts on testing:

• First, I need to test the baselines for both normal-projects and projects-with-cache:
  • I should be able to load Workflow X (either accessing the Classify WF X URL directly, or coming from the home page)
  • I should be able to switch to Workflow Y (either by the Classify link, or going to the home page)
  • (can't test on localhost) I should be able to go from WF X on Davy Notebooks to Talk and then Back.
• Advanced Tests I'm considering:
  • Does hydrateStore's catch properly handle borked snapshots? (e.g. snapshot too huge, corrupted JSON, etc) This should gracefully proceed as if the initial snapshot was just {}
  • Is my assumption that the code first checks sessionStorage, then checks for fresh data from Panoptes correct? Is there any scenario where the opposite might happen? (Sanity check: nope, reading the code, I can't see how that could happen)
  • What happens when the user has a slow connection? (presumably: Classifier loads "normally", user proceeds classifying, then option 1: if there's a newer version of WF X, then suddenly BLOOP the Classifier 'jolts' with an update that resets classification, or option 2: if there's no change to WF X, then user proceeds without noticing anything)
  • If a project owner is editing WF X on Project Builder on another tab, will the changes appear on the Classify WF X tab when the PO changes tab/refreshes? (Note: probably can't test this on projects-with-cache since I don't have any on staging, but I CAN test this for normal-projects to ensure that, if a normal user refreshes a tab that they've kept open for days, the newer WF X data will override the older version in sessionStorage.)

Code read notes: (mostly rambling, no blocking comments)

• The logic branching for normal-project vs project-with-cached-data (i.e. where if (cachePanoptesData) ... resides) previously happened in ClassifierContainer -> useHydratedStore
• This branching now occurs in ClassifierContainer -> useHydratedStore -> useSessionStorage and useSavedSnapshot
• This works fine, and the Readme covers most of what devs need to know, but it's a bit sad that since we have more layers now, we're losing some of the clarity of the previous version (though to be fair, the complexity was hidden behind a lot of the mst-persist dependency, so eh)
• I can follow the general idea of how this works, but what's boggling me is that useSavedSnapshot() is asynchronous, so I can't spot all the ways this might break when I run the flow in my head.

Status

Testing in progress, check back with me on Thursday.

I think I should tag in Mark or Delilah in for help with additional tests, since they caught that issue where... if a user goes from Classifier (Workflow X) to Home to Classifier (Workflow Y)... the classifier borks? (EDIT: It was issue #2891, and the bug was that the "prioritised workflow" logic branch getting caught with the normal branches.) Anyway, I'm just really cautious about refactors to the Stores since then, and extra eyes would be good.

[shaunanoordin]

Blargh! I keep running into this Unhandled Runtime Error when I try to load a Classify page for any Workflow X on a normal-project or a project-with-cache

master (b5e71f5) looks fine and non-Classify pages on this branch look fine, but honestly there's probably something borked in my settings or my setup. I'll try a full yarn panic and bootstrap next time, so check back with me on Thursday. Might need to check my node settings too, since I know my nvm keeps resetting to v14 due to some shenanigans with the aliasing.

[eatyourgreens]

@shaunanoordin I'm also seeing a crash when I try the test link for Davy Notebooks, with a production build. This branch is quite old, so could have been broken by recent changes.

[eatyourgreens]

@shaunanoordin #2895 introduced this line, which crashes when classifierStore is undefined. I'll push a fix to this branch, which should get the classifier working again.

front-end-monorepo/packages/lib-classifier/src/components/Classifier/ClassifierContainer.js

Line 75 in 617045a

const { classifications, subjects, workflows } = classifierStore

[shaunanoordin]

Confirming that I can now load the Classifier on this branch, so testing has resumed! 👍

[shaunanoordin]

Wait, did I speak too soon? Tests were running fine for my random test projects, but now that I'm testing with Davy Notebooks I'm encountering errors. Here are my notes so far:

Testing

Tested on localhost with macOS + Chrome 99. I'm using new Incognito Mode tabs on each test/test step to maintain Session Storage freshness.

Baseline tests: normal-projects

• Project: random test project with multiple WFs to choose from
• As a volunteer, I can view the project home page
• I can view the Classify page of any WF X (and submit Classifications) by either 1. choosing from the home page, 2. choosing from the Classify link's WF menu, or 3. typing in the URL directly
• From the Classify page for WF X, I can switch to WF Y by either 1. going back to the home page and choosing WF Y, or 2. choosing WF Y from the Classify link's WF menu.
• As a dev, I can confirm that the Classifier store is NOT stored in Session Storage.

Feature tests: projects-with-cache

• Project: Davy Notebooks (production)
• ERROR: when attempting to choose any WF from the home page, e.g. https://local.zooniverse.org:3000/projects/humphrydavy/davy-notebooks-project/classify/workflow/20873?env=production&demo=true, I receive the error TypeError: Cannot read properties of undefined (reading 'subjectViewer')
  • I can confirm that this error doesn't exist on master, nor on zooniverse.org

Above: the error message. This is a fresh Incognito Mode tab, so there's no saved state in Session Storage.

[shaunanoordin]

Update:

• The error triggers even on the /classify with no WF ID.
• The error triggers on HMS NHS too
• New URLs where I see the same issue:
  • https://local.zooniverse.org:3000/projects/humphrydavy/davy-notebooks-project/classify?env=production&demo=true
  • https://local.zooniverse.org:3000/projects/msalmon/hms-nhs-the-nautical-health-service/classify?env=production&demo=true

Good news, other projects seem fine, e.g. Galaxy Zoo Weird and Wonderful and TESS, so we can narrow down the debug to anything that uses the Panoptes cache (Classifier Store in Session Storage) option, and it's specific to this PR (again, master is fine)

[eatyourgreens]

My mistake. setLoaded(true) was outside an if block when it should have been inside it. The full error was classifierStore is undefined which, yeah, would definitely be very bad for the classifier. It's fixed now.

I really need a good strategy for testing that top-level classifier container. React Testing Library cautions against testing hooks in isolation, but I wonder if useHydratedStore should have tests anyway eg. to catch that loaded state was being set before the store had indeed loaded.

[eatyourgreens]

I've written an ADR to go along with this:

#2946

[shaunanoordin]

PR Review (Update)

Following up on the initial review and first update

I did new code read to catch up with the latest changes.

• We've got a latch in useSessionStorage, with the loaded state var ensuring the Classifier Store only gets an "onSnapshot" listener one time. (This was added after the last review, right?)
• There's a small existence check added in checkForProgress. Nice.
• I think the major Classifier-crashing bugs I was encountering before were fixed in 2968 (which fixed the undefined classifierStore at the onLoad hook), and this PR has been rebased on top of that?

I might be missing some other fixes stemming from other PRs, but long story short, code read looks good, and testing looks fine 👍

Testing

Tested on localhost with macOS + Chrome 99. I'm using new Incognito Mode tabs on each test/test step to maintain Session Storage freshness.

Baseline tests: normal-projects

• Project: random test project with multiple WFs to choose from
• As a volunteer, I can view the project home page
• I can view the Classify page of any WF X (and submit Classifications) by either 1. choosing from the home page, 2. choosing from the Classify link's WF menu, or 3. typing in the URL directly
• From the Classify page for WF X, I can switch to WF Y by either 1. going back to the home page and choosing WF Y, or 2. choosing WF Y from the Classify link's WF menu.
• As a dev, I can confirm that the Classifier store is NOT stored in Session Storage.

Feature tests: projects-with-cache

• Project: Davy Notebooks (production)
• Same behaviour tests as above
• As a dev, I can confirm that the Classifier store IS stored in Session Storage.

Status

I have no additional comments to make on the code, and my previous concerns with the crashing manual tests have been addressed. 👍 This PR is now LGTM, and I'll wait for @goplayoutside3 's notes on this PR

(PS: Sorry for the Saturday review, my Friday evening work plans went sideways so I'm playing catch up.)

[goplayoutside3]

PR Review

I can confirm the same UX testing as Shaun. I'm able to load the example projects.

I'm requesting changes because I think the bug fix I flagged should be in its own PR.

I also think it's confusing that cachePanoptesData and enableStorage are the same variable, but named differently across hooks and the Readme. I recommend picking one or even consistently naming it "enableSessionStorage" because that's the intended purpose.

Overall, it feels weird to have the check for cachePanoptesData in every ClassifierContainer hook instead of one check in ClassifierContainer. For example, if cachePanoptesData is false, simply init the store from scratch, and if cachePanoptesData is true, then cascade all three custom hooks to handle the sessionStorage portion.

In constrast, this is the data flow in this PR:

useHydratedStore calls useStore under the hood. But useStore requires an initialState. initialState is returned from useSavedSnapshot. If cachePanoptesData is false, initialState is simply an empty object {}. Only then is useStore called to init classifierStore. Finally, useSessionStorage is called anyway regardless if we need to use sessionStorage.

I think that cascade of function calls is confusing for the goal of optional sessionStorage. If it remains, I think it needs to be explicitly outlined in ClassifierContainer via readme or inline comments.

[eatyourgreens]

Hooks can’t be conditional, I’m afraid, so wrapping them in a check for cachePanoptesData will throw a React error.

[eatyourgreens]

@goplayoutside3 I've consolidated the hooks into a single hook. The hook code gets a bit more complicated, but that allows us to use conditional logic without breaking the rules of hooks.

EDIT: I realised that the storage here doesn't really need asynchronous read/write. That removed a whole bunch of useEffect/useState complexity from the hook code.

[eatyourgreens]

Updated to rename hook args, for clarity, after I looked up the arguments for create in the MST API:
https://mobx-state-tree.js.org/API/interfaces/imodeltype#create

[goplayoutside3]

Code read looks good! I think the hooks/Readme is clearer and useHydratedStore is easier to understand. ADR 42 also accurately reflects this refactor.

I can confirm the UX tested earlier on this branch, and projects with prioritized workflow(s) have their classifier store in sessionStorage and regular project's classifier store do not.