Classifier: Refactor ClassifierContainer hooks #2793

eatyourgreens · 2022-02-07T10:37:01Z

Add a hooks directory to the Classifier component. Import the useStore and useWorkflowSnapshot hooks from that directory.
Explicitly declare SWR options in the useWorkflowSnapshot hook.
Add a useHydratedStore hook, which hydrates the store from session storage when cachePanoptesData is set. Otherwise, it just returns true.
Add a useEffect hook to clean up the store and set up volatile callbacks for the parent page after hydration has finished.
Add a store.startClassification() action, to replace the subjects.setActiveSubject(subjects.active?.id) hack.
Additional console logging when the classifier mounts and creates the store.

This PR includes #2792. It shouldn't change anything, just refactor the hooks to make the classifier component easier to read and debug.

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?

goplayoutside3

PR Review

I tested this refactor PR by making real classifications to the i-fancy-cats staging project, and I made demo classifications to the live FEM project Corresponding with Quakers. Functionality is unchanging 👍

Classifier and ClassifierContainer do not have any test files, which is on our radar as a team, so in an effort to add better tests and documentation to lib-classifier (like your question in #2809), can a spec file be added for each custom hook in this PR? I’m thinking a unit test similar to useStores, or if mocking workflows and stores is overkill, then a README for each custom hook will be super helpful going forward.

packages/lib-classifier/src/components/Classifier/ClassifierContainer.js

goplayoutside3 · 2022-02-15T19:02:59Z

packages/lib-classifier/src/components/Classifier/Classifier.js

@@ -37,6 +37,7 @@ export default function Classifier({
  useEffect(function onURLChange() {
    const { workflows } = classifierStore
    if (workflowID) {
+      console.log('starting new subject queue', { workflowID, subjectSetID, subjectID })


Suggested change

console.log('starting new subject queue', { workflowID, subjectSetID, subjectID })

// console.log('starting new subject queue', { workflowID, subjectSetID, subjectID })

Console logs in this PR are helpful for classifier debugging, but the ones that log data will be noisy if these are committed to master. I'm suggesting commenting them out until they're specifically used for debugging locally.

Console logs are also recorded in Sentry. I've added these so that we can better use Sentry to debug problems on Davy Notebooks and HMS NHS.

I'd prefer to leave them in until we're sure that those projects are working properly.

goplayoutside3 · 2022-02-15T19:05:30Z

packages/lib-classifier/src/components/Classifier/Classifier.js

@@ -51,9 +52,10 @@ export default function Classifier({
    if (workflowSnapshot) {
      // pass the subjectSetID prop into the store as part of the new workflow data
      workflowSnapshot.subjectSet = subjectSetID
+      console.log('Refreshing workflow snapshot', workflowSnapshot.id)


Suggested change

console.log('Refreshing workflow snapshot', workflowSnapshot.id)

// console.log('Refreshing workflow snapshot', workflowSnapshot.id)

eatyourgreens · 2022-02-15T20:26:38Z

Testing hooks in isolation isn't recommended. Better to test them in the component that uses them.

https://react-hooks-testing-library.com/

Here, that would involve mounting and rendering the classifier, maybe with mocked local storage.

goplayoutside3 · 2022-02-15T22:09:52Z

Testing hooks in isolation isn't recommended. Better to test them in the component that uses them.

Makes sense, and in that case can a README be added to the /hooks folder? For example, it should explain the purpose and role of mst-persist in useHydratedStore and how SWR is important to useWorkflowSnapshot. Keeping the team up to speed on data flow in Classifier and ClassifierContainer will be immensely helpful to reviewing bug fixes, especially because those component files are missing tests.

No rush! This is a refactor PR.

eatyourgreens · 2022-02-17T14:03:48Z

I've added a short Readme for the hooks. They're mostly wrappers for external libraries, so I've included links to those libraries too.

goplayoutside3

I've added a short Readme for the hooks. They're mostly wrappers for external libraries, so I've included links to those libraries too.

Looks good thanks!

- Add a `hooks` directory to the `Classifier` component. Import the `useStore` and `useWorkflowSnapshot` hooks from that directory. - Explicitly declare `SWR` options in the the `useWorkflowSnapshot` hook.

Move the store hydration and initialisation code into a `useHydratedStore` hook.

Move store clean-up into `ClassifierContainer`. `useHydratedStore` hydrates the store but doesn't try to alter it after loading it.

`store.startClassification` allows us to start a new classification automatically, when the subject queue advances, or manually, after loading a workflow from Panoptes.

Co-authored-by: Delilah C. <23665803+goplayoutside3@users.noreply.github.com>

eatyourgreens added the refactor Refactoring existing code label Feb 7, 2022

eatyourgreens requested a review from a team February 7, 2022 10:37

eatyourgreens force-pushed the refactor-classifier-hooks branch from 3df2d43 to 65121a0 Compare February 7, 2022 10:37

eatyourgreens mentioned this pull request Feb 7, 2022

Classifier: declare SWR options #2792

Closed

18 tasks

eatyourgreens changed the title ~~Classifier: Refactor useStore and useWorkflowSnapshot~~ Classifier: Refactor ClassifierContainer hooks Feb 7, 2022

eatyourgreens force-pushed the refactor-classifier-hooks branch 2 times, most recently from 3998f30 to 877bb0a Compare February 7, 2022 13:07

eatyourgreens force-pushed the refactor-classifier-hooks branch 4 times, most recently from 18a958c to 852f74f Compare February 8, 2022 13:09

eatyourgreens mentioned this pull request Feb 8, 2022

Davy Notebooks crashes when you change workflow #2799

Closed

eatyourgreens force-pushed the refactor-classifier-hooks branch from c954986 to b51ab8c Compare February 9, 2022 20:41

eatyourgreens mentioned this pull request Feb 10, 2022

Error switching to different workflow from classification in progress #2809

Closed

eatyourgreens force-pushed the refactor-classifier-hooks branch from b51ab8c to ecf8fe6 Compare February 10, 2022 11:30

goplayoutside3 self-assigned this Feb 14, 2022

goplayoutside3 reviewed Feb 15, 2022

View reviewed changes

eatyourgreens mentioned this pull request Feb 15, 2022

Classifier: fix duplicate subjects (with ordered subject selection.) #2819

Merged

18 tasks

eatyourgreens force-pushed the refactor-classifier-hooks branch from d245960 to db28fa5 Compare February 17, 2022 12:19

goplayoutside3 approved these changes Feb 18, 2022

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Feb 18, 2022

eatyourgreens added 5 commits February 18, 2022 05:07

Refactor useStore and useWorkflowSnapshot

d5c3ca6

- Add a `hooks` directory to the `Classifier` component. Import the `useStore` and `useWorkflowSnapshot` hooks from that directory. - Explicitly declare `SWR` options in the the `useWorkflowSnapshot` hook.

Add useHydratedStore

6968fc3

Move the store hydration and initialisation code into a `useHydratedStore` hook.

Separate out classifier callbacks from hydration

9ad3b81

Clean up useHydratedStore

ae7c000

Move store clean-up into `ClassifierContainer`. `useHydratedStore` hydrates the store but doesn't try to alter it after loading it.

Add store.startClassification

0d8ba01

`store.startClassification` allows us to start a new classification automatically, when the subject queue advances, or manually, after loading a workflow from Panoptes.

eatyourgreens and others added 4 commits February 18, 2022 05:07

Add debugging logs to Classifier hooks

179b075

Remove unused i18n

5a014c5

Co-authored-by: Delilah C. <23665803+goplayoutside3@users.noreply.github.com>

whitespace

f164017

Co-authored-by: Delilah C. <23665803+goplayoutside3@users.noreply.github.com>

Add a hooks Readme

34d56b5

eatyourgreens force-pushed the refactor-classifier-hooks branch from 9e3235c to 34d56b5 Compare February 18, 2022 05:07

eatyourgreens merged commit 88bc3b8 into master Feb 18, 2022

eatyourgreens deleted the refactor-classifier-hooks branch February 18, 2022 05:21

eatyourgreens mentioned this pull request Feb 18, 2022

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

Merged

18 tasks

goplayoutside3 mentioned this pull request Feb 25, 2022

Navigating from classifier, to project home, back to classifier - task component not rendered #2891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classifier: Refactor ClassifierContainer hooks #2793

Classifier: Refactor ClassifierContainer hooks #2793

eatyourgreens commented Feb 7, 2022 •

edited

Loading

goplayoutside3 left a comment

goplayoutside3 Feb 15, 2022

eatyourgreens Feb 15, 2022

goplayoutside3 Feb 15, 2022

eatyourgreens commented Feb 15, 2022

goplayoutside3 commented Feb 15, 2022

eatyourgreens commented Feb 17, 2022

goplayoutside3 left a comment

	console.log('starting new subject queue', { workflowID, subjectSetID, subjectID })
	// console.log('starting new subject queue', { workflowID, subjectSetID, subjectID })

	console.log('Refreshing workflow snapshot', workflowSnapshot.id)
	// console.log('Refreshing workflow snapshot', workflowSnapshot.id)

Classifier: Refactor ClassifierContainer hooks #2793

Classifier: Refactor ClassifierContainer hooks #2793

Conversation

eatyourgreens commented Feb 7, 2022 • edited Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

goplayoutside3 left a comment

Choose a reason for hiding this comment

PR Review

goplayoutside3 Feb 15, 2022

Choose a reason for hiding this comment

eatyourgreens Feb 15, 2022

Choose a reason for hiding this comment

goplayoutside3 Feb 15, 2022

Choose a reason for hiding this comment

eatyourgreens commented Feb 15, 2022

goplayoutside3 commented Feb 15, 2022

eatyourgreens commented Feb 17, 2022

goplayoutside3 left a comment

Choose a reason for hiding this comment

eatyourgreens commented Feb 7, 2022 •

edited

Loading