VV: Display the VV in the Classifier #6457

kieftrav · 2024-11-13T16:11:01Z

Package

lib-classifier and lib-subject-viewers

Describe your changes

Pull in the experimental_tools flag for the volumetricViewer to the Project Store
ProjectStore has a view that returns a boolean to indicate if the active project is a VolumetricViewer project
SubjectViewer fetches the boolean and displays the correct Viewer based on that Boolean
Create SubjectViewer spec that tests for the suspense boundary and the correct loading of the VV
Remove ProtoViewer after incorporating all details into VV
Create /hooks folder with newly created useSubjectJSON.js to fetch the Subject JSON

How to Review

Storybook should load up the VV with no issues and display the 4x4x4 cube
Running lib-classifier locally should display the VV with the 4x4x4 cube
Running app-project locally should display the VV with the 4x4x4 cube
All tests should pass

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

New Feature

The PR creator has listed user actions to use when testing the new feature
Unit tests are included for the new feature
A storybook story has been created or updated
- Example of SlideTutorial component with docgen comments, and its story

coveralls · 2024-11-13T16:18:41Z

coverage: 77.793% (-0.05%) from 77.843%
when pulling 83c7f91 on vv-display-classifier
into 74d5ede on master.

goplayoutside3

In this PR, I think there's a bit of a misunderstanding about using custom hooks, so I left a detailed comment with documentation and reference to examples in this repo. The current pattern of useSubjectJSON() in the VolumetricViewer.js is a blocker to merging this PR, so please see my recommendations on how to change it, and let me know if you'd like to have a call to discuss.

goplayoutside3 · 2024-11-14T14:17:46Z

packages/lib-classifier/src/store/ProjectStore.js

+  .views(self => ({
+    get isVolumetricViewer() {
+      return (isValidReference(() => self.active))
+        ? self.active.experimental_tools.includes('volumetricViewer')
+        : false
+    }
+  }))


This works and I can see the VolumetricViewer component when I run this branch locally. In our Tues meeting I recommended this view be on the Project store model, which can be confusing because there's Project.js and ProjectStore.js. I mentioned there are already two views on the Project store model, and those are in Project.js.

At this point I think it's half dozen of one and six of the other, but just wanted to be clear about what I recommended Tuesday. In ProjectStore.js, you have to use isValidReference() to grab the active project, whereas if the view is in Project.js, you just grab the active project via the storeMapper(). An example of the latter is get display_name() below (just an FYI, feel free to keep the view in ProjectStore.js if you prefer):

front-end-monorepo/packages/lib-classifier/src/store/Project/Project.js

Lines 24 to 26 in fbab1d8

get display_name() {

return self.strings.get('display_name')

},

The presentational component SlideTutorialContainer uses project.display_name view:

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

Lines 12 to 14 in fbab1d8

projects: {

active: project

},

Got this moved over to the proper file in latest commit - thanks for the clarification!

goplayoutside3 · 2024-11-14T14:41:11Z

packages/lib-subject-viewers/src/VolumetricViewer/VolumetricViewer.js

  useEffect(() => {
-    if (subjectData !== '') {
-      setData(Buffer.from(subjectData, 'base64'))
-    } else if (subjectUrl !== '') {
-      fetch(subjectUrl)
-        .then((res) => res.json())
-        .then((data) => {
-          setData(Buffer.from(data, 'base64'))
-        })
-    } else {
-      console.log('No data to display')
-    }
-  }, [])
+    useSubjectJSON({ setSubjectData, subject })
+  }, [subject])


It's a bit difficult to see from the diff, but this is the pattern you've got here:

useEffect(() => { useSubjectJSON({ setSubjectData, subject }) }, [subject])

On first look, this is a custom hook inside useEffect which goes against the React Rules of Hooks "Do not call Hooks inside functions passed to useMemo, useReducer, or useEffect." However, looking closer at your function named useSubjectJSON() in lib-subject-viewers, it's not actually a hook.

useSubjectJSON() in lib-classifier utilizes useEffect and useState to fetch and return subject data to the viewer. It's a custom React hook and allowed the "use" naming convention because useEffect and useState are inside the useSubjectJSON() function.

In this PR, you've got useEffect and useState inside VolumetricViewer.js instead of inside useSubjectJSON(), which is just a fetcher function.

Is this pattern intentional? Let me know if you'd like to have a call to discuss custom hooks.

Overall, I recommend:

Naming your fetcher function in lib-subject-viewers something specific to volumetric data like useVolumetricSubject().

Pass onError, onReady, and subject to a custom hook useVolumetricSubject() exactly like it's done below for JSONDataViewer. The custom hook useVolumetricSubject() then returns subjectData, and error. This pattern would clean up the code in VolumetricViewer.js, removing the need for two useEffect functions, and the need to store subjectData and error status in a useState.

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

Line 45 in fbab1d8

const { data: jsonData, loading, error, type, viewer } = useSubjectJSON({ onError, onReady, subject})

Got this incorporated as well - I appreciate the focus on the hook's interface within the VolumetricViewer component. That helped me refactor/isolate where the state should live.

kieftrav · 2024-11-19T18:17:32Z

@goplayoutside3 - got the changes suggested above incorporated and incorporated a couple small changes based on our conversation during today's Brain meeting.

goplayoutside3

Looking much better, thanks for the update! I'm able to view the volumetric viewer test project while running app-project or lib-classifier locally. No other FEM subject viewers are affected 👍

I'm going to approve, but see my notes in the SubjectViewer.spec.js file. The unit tests need a bit of reworking before this PR can be merged.

goplayoutside3 · 2024-11-20T15:39:26Z

...ages/lib-classifier/src/components/Classifier/components/SubjectViewer/SubjectViewer.spec.js

-
-    const wrapper = shallow(<SubjectViewer subjectQueueState={asyncStates.success} subject={{ viewer: 'variableStar', viewerConfiguration }} />)
-    expect(wrapper.find(JSONDataViewer).props().viewerConfiguration).to.deep.equal(viewerConfiguration)
+  it('should render the VolumetricViewer if the subject store successfully loads', function () {


Suggested change

it('should render the VolumetricViewer if the subject store successfully loads', function () {

it('should render the VolumetricViewer if isVolumetric = true', function () {

This test is specific to a volumetric project. You've deleted the generic unit test for 'if the subject store successfully loads'.

Yes - this was because the spec was originally in Enzyme and mounting/rendering sub-components gets pretty dicy when converting to RTL... So, my thought was to make a more "robust" test of the SubjectViewer that handles the async nature of the VolumetricViewer because, if it can handle that, it should be able to render the default Viewer. I admit it compromises completeness of test coverage (handling the rendering of the non-async viewers), but the test I added is meant to make up for that with it being the more "complex" scenario due to its async nature.

goplayoutside3 · 2024-11-20T15:45:27Z

...ages/lib-classifier/src/components/Classifier/components/SubjectViewer/SubjectViewer.spec.js

+    expect(screen.getByText('Suspense boundary')).to.exist()
+    expect(screen.findByTestId('subject-viewer-volumetric')).to.exist()


The expectation on line 38 is a false positive. I can change that line to findByTestId('subject-viewer-volumetriccccc').to.exist() and it will still be true. See the documented Issue about findBy queries: #6383

This unit test needs reworking before this PR can be merged.

Great catch! Fixed with adding async in the spec and a proper return element that doesn't dive into WebGL/Canvas context rendering in the testrunner.

kieftrav · 2024-11-20T19:41:11Z

@goplayoutside3 - fixed the broken spec and explained the approach to writing that spec. If you'd like me to add another spec for the non-volumetric viewers, I can timebox getting one to render without throwing up errors given the Enzyme => RTL conversion removes the shallow-mounting benefit that Enzyme had.

goplayoutside3 · 2024-11-20T22:19:23Z

The SubjectViewer.js tests are now failing due to Uncaught Error: Rendered fewer hooks than expected and done() called multiple times in test <Component > SubjectViewer. See the failed ci run here: https://github.com/zooniverse/front-end-monorepo/actions/runs/11940429479/job/33283064064. Please take a look at the failing isVolumetricViewer test.
Re: Writing RTL tests for the case when "it should render the viewer if the subject store is ready" and not a volumetric project:

Taking a closer look at SubjectViewer.js, the props do not have default props except for subjectQueueState. Therefore, if you don't pass all of the props to SubjectViewer in a mocked env like a test suite, the component won't behave as expected. I'm not suggesting adding default props to SubjectViewer because doing so would affect all viewer components.

While we don't expect there to be a Viewer component when subjectQueueState is "initialized" or "loading", we do expect there to be a viewer component when subjectQueueState is a "success" and a subject is available via the store. Here's an example of how to pass all necessary props to SubjectViewer in a test suite:
```
  it('should render the viewer if the subject store is ready', async function () {
    render(
      <SubjectViewer
        subjectQueueState={asyncStates.success}
        subjectReadyState={asyncStates.success}
        isVolumetricViewer={false}
        onSubjectReady={() => {}}
        onError={() => {}}
        subject={{
          id: '1234',
          viewer: 'singleImage'
        }}
      />
    )
  })
```

The only thing missing from this setup ^ is a mocked Mobx store because most of the viewer components have a "connector" wrapper that looks to the store for more data. The store is often mocked in Storybook and then used in a spec file, but here's one example where the store is mocked directly in a spec:

front-end-monorepo/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/InteractionLayer/components/PreviousMarks/PreviousMarks.spec.js

Lines 60 to 70 in b5af532

    
           function withStore(store) { 
        
             return function Wrapper({ children }) { 
        
               return ( 
        
                 <Provider classifierStore={store}> 
        
                   <svg> 
        
                     {children} 
        
                   </svg> 
        
                 </Provider> 
        
               ) 
        
             } 
        
           }

You're welcome to continue with just one test for isVolumetricViewer = true in this PR and in that case I will follow-up with a PR that implements the additional "it should render the viewer if the subject store is ready" case. It could also be a good learning opportunity to include both cases in this PR (non volumetric and is volumetric). Let me know what you decide and please re-request my review when all CI tests pass.

CI tests are failing due to changes in SubjectViewer.spec.js

kieftrav · 2024-11-21T14:44:52Z

Hey Delilah - quick summary of the 2 most recent commits:

Fixed the broken spec that caused the test suite to fail.
Re-implemented the spec that renders the Viewer when the SubjectStore is ready
Re-requesting review as requested

goplayoutside3

Thanks!

kieftrav marked this pull request as draft November 13, 2024 16:11

kieftrav force-pushed the vv-display-classifier branch from eeab260 to ea41bde Compare November 13, 2024 16:12

kieftrav changed the title ~~WIP: Display VV in project.~~ DRAFT: Display VV in project. Nov 13, 2024

kieftrav force-pushed the vv-display-classifier branch from ea41bde to 8acf455 Compare November 13, 2024 16:58

kieftrav marked this pull request as ready for review November 13, 2024 17:05

kieftrav requested a review from goplayoutside3 November 13, 2024 17:05

kieftrav changed the title ~~DRAFT: Display VV in project.~~ VV: Display the VV in the Classifier Nov 13, 2024

goplayoutside3 reviewed Nov 14, 2024

View reviewed changes

goplayoutside3 self-assigned this Nov 14, 2024

kieftrav force-pushed the vv-display-classifier branch 3 times, most recently from c01f534 to a6158bd Compare November 19, 2024 18:09

kieftrav requested a review from goplayoutside3 November 19, 2024 18:16

goplayoutside3 previously approved these changes Nov 20, 2024

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Nov 20, 2024

kieftrav force-pushed the vv-display-classifier branch from a6158bd to ece899d Compare November 20, 2024 19:31

kieftrav force-pushed the vv-display-classifier branch from 0070da9 to 679f7d5 Compare November 21, 2024 13:52

kieftrav requested a review from goplayoutside3 November 21, 2024 14:44

goplayoutside3 approved these changes Nov 21, 2024

View reviewed changes

kieftrav added 5 commits November 21, 2024 15:13

Display VV in Classifier

e5f7b69

Incorporate PR feedback

1d0667d

Fix spec

a7cd986

Reimplement rendering viewer if the subject store is ready.

9378b1b

Fix failing spec.

83c7f91

kieftrav force-pushed the vv-display-classifier branch from 679f7d5 to 83c7f91 Compare November 21, 2024 21:13

kieftrav merged commit db17bc5 into master Nov 21, 2024
7 checks passed

kieftrav deleted the vv-display-classifier branch November 21, 2024 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VV: Display the VV in the Classifier #6457

VV: Display the VV in the Classifier #6457

kieftrav commented Nov 13, 2024 •

edited

Loading

coveralls commented Nov 13, 2024 •

edited

Loading

goplayoutside3 left a comment

goplayoutside3 Nov 14, 2024

kieftrav Nov 19, 2024

goplayoutside3 Nov 14, 2024

kieftrav Nov 19, 2024

kieftrav commented Nov 19, 2024

goplayoutside3 left a comment •

edited

Loading

goplayoutside3 Nov 20, 2024

kieftrav Nov 20, 2024

goplayoutside3 Nov 20, 2024

kieftrav Nov 20, 2024

kieftrav commented Nov 20, 2024

goplayoutside3 commented Nov 20, 2024

kieftrav commented Nov 21, 2024

goplayoutside3 left a comment

	get display_name() {
	return self.strings.get('display_name')
	},

	it('should render the VolumetricViewer if the subject store successfully loads', function () {
	it('should render the VolumetricViewer if isVolumetric = true', function () {

		expect(screen.getByText('Suspense boundary')).to.exist()
		expect(screen.findByTestId('subject-viewer-volumetric')).to.exist()

VV: Display the VV in the Classifier #6457

VV: Display the VV in the Classifier #6457

Conversation

kieftrav commented Nov 13, 2024 • edited Loading

Package

Describe your changes

How to Review

Checklist

General

General UX

New Feature

coveralls commented Nov 13, 2024 • edited Loading

goplayoutside3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieftrav commented Nov 19, 2024

goplayoutside3 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kieftrav commented Nov 20, 2024

goplayoutside3 commented Nov 20, 2024

kieftrav commented Nov 21, 2024

goplayoutside3 left a comment

Choose a reason for hiding this comment

kieftrav commented Nov 13, 2024 •

edited

Loading

coveralls commented Nov 13, 2024 •

edited

Loading

goplayoutside3 left a comment •

edited

Loading