Viewers: Redux: actions have inconsistent payloads and/or huge payloads, state has circular references, ReduxDevTools crashes

Created on 7 Oct 2019 · 6Comments · Source: OHIF/Viewers

Bug Report

Describe the Bug

Recently I've encountered the following problems in how Redux is used in the OHIF viewer and libraries:

Some of the Redux actions dispatched in the viewer have inconsistent payloads. That is, an action of a given type may have data layout completely different from another action of the same type. This is unsafe and violates the very basic principles of Redux: clients consuming these actions (whether OHIF or third party) cannot count on receiving a particular piece of data anymore. This leads to frustration and breakage.
Some of the actions have payloads that are way too big for any real-world use
Some actions result in Redux state having circular references in its object layout
Redux Dev Tools are slow and may crash unpredictably

What steps can we follow to reproduce the bug?

Launch a chromium-based browser and install latest version of Redux Dev Tools (2.17.0 as of today)
Open Chrome Dev Tools and switch to "Redux" tab
Open, for example, https://viewer.ohif.org/viewer/1.2.826.0.13854362241694438965858641723883466450351448

Patient Name: Dummy
MRN: 716713622818558421526494572171041020891020924597
Study: 1.2.826.0.13854362241694438965858641723883466450351448

Click on first VIEWPORT::SET action in actions sidebar. Click "Action" tab. You will see that this action dispathes the following object:

{
type: 'VIEWPORT::SET',
viewportIndex: 0,
data: {
  displaySetInstanceUid: '7d860e86-cb2f-2758-4a55-a8815e93e311',
  studyInstanceUid: '1.2.826.0.13854362241694438965858641723883466450351448',
  currentImageIdIndex: 0,
  sopInstanceUid: '1.2.826.0.13567014206729755965107989533871182029618726'
}
}

Click on next VIEWPORT::SET action in actions sidebar. You will see that this action has completely different data:

```js
{
type: 'VIEWPORT::SET',
viewportIndex: 0,
data: {
plugin: "cornerstone",
dom: "<120k lines of json which make my computer hang for a minute>"
}
}


- Further, scroll slices in the viewport, try to drag the "SEG" series onto the viewport. `RESET_LABELLING_AND_CONTEXT` and more `VIEWPORT:SET` actions will be dispatched. At some point Redux Dev Tools will stop responding to user interaction and the popup "Redux DevTools has crashed" will appear:

![Screenshot of the popup "Redux DevTools has crashed"](https://user-images.githubusercontent.com/9403403/66325834-3ba2cf00-e928-11e9-8bfd-4101a82e5fc8.png)

 - If one is quick enough, before dev tools crash, they may be able to catch the following pink elephants in state snapshots on "State" tab (note `'[CIRCULAR]'`):

 ```js
{
  ...
  viewports: '[CIRCULAR]',
  ...
}

 ```

```js
{
  ...
  viewports: {
    activeViewportIndex: 0,
    layout: {
      viewports: [
        {
          height: '100%',
          width: '100%',
          plugin: 'cornerstone'
        }
      ]
    },
    viewportSpecificData: {
      '0': {
        plugin: 'cornerstone',
        dom: '[CIRCULAR]',
        displaySetInstanceUid: '70ade2ad-d07e-c252-49c8-0d212aacba16',
        studyInstanceUid: '1.2.826.0.13854362241694438965858641723883466450351448',
        currentImageIdIndex: 0,
        sopInstanceUid: '1.2.826.0.11521735486436309604024223829282084628708383',
        seriesDate: '20190129',
        seriesTime: '153943',
        seriesInstanceUid: '1.2.826.0.53608788865337544170015089925954063493496832',
        seriesNumber: 1001,
        seriesDescription: 'White matter hyperintensity segmentation',
        numImageFrames: 48,
        modality: 'SEG',
        isMultiFrame: true,
        instanceNumber: 1,
        sopClassUids: [
          '1.2.840.10008.5.1.4.1.1.66.4'
        ],
        isClip: true
      }
    }
  },
  ...
}

Possible resolution strategies

Consider adopting a stable action layout. Action creators shoud be made more type safe. It is not enough to just pass data to action creator and to insert data into the action object as is. Every field of data should be specified separately to ensure that there is no divergence in data type. Example:

function setViewportData(data) {
  return  {
    type: 'VIEWPORT:SET',
    // data can be anything
    data
  };
}

dispatch(setViewportData({ dom: myEntireDom })  // uh-oh! Where is my studyId?

function setViewportData(studyId, seriesId) {
  return  {
    type: 'VIEWPORT:SET',
    // data can only be an object containing studyId, seriesId. both can be anything
    data: {
      studyId,
      seriesId,
    }
  };
}

dispatch(setViewportData({ studyId: '1.2.3', seriesId: undefined })) // better, but still

Consider adopting Flux Standard Action pattern

Consider switching relevant parts of the application and libraries to Typescript (see e.g. @babel/preset-typescript). Introducing types and type-safe actions, e.g. via typescript-fsa, will be a major improvement in terms of usability of these actions and of 3rdparty-developer experience. Example:

interface SetViewportDataParams {
  studyId: string;
  seriesId: string;
}

function setViewportData({ studyId, seriesId }: SetViewportDataParams) {
  return  {
    type: 'VIEWPORT:SET',
    // data can only be an object containing studyId and seriesId, both are strings
    data: { 
      studyId,
      seriesId,
    }
  };
}

dispatch(setViewportData({ dom: myEntireDom })) // compiler error

dispatch(setViewportData({ studyId: '1.2.3', seriesId: undefined })) // compiler error

dispatch(setViewportData({ studyId: '0.1.23', seriesId: '4.5.678' })) // okay

Consider using Redux Dev Tools more during development process in order to verify actions payloads and state changes. Developers typically expect Redux Dev Tools to be functioning on any project. It is frustrating when they are not: "What do I do now? console.log everything?"
Consider adding more unit tests for component logic
Should DOM objects be a part of the action payload? I think this particular dom entry is way to big to be useful
Consider making eslint config more strict. There are eslint rules that may help to catch bugs in redux

Bug

Source

ivan-aksamentov

👍1

All 6 comments

Here are some additional findings regarding how setViewportSpecificData is dispatched.

This is where dom thing is dispatched instead of usual data:
https://github.com/OHIF/Viewers/blob/b59c8c277af38ab601c2f3baa5a0cef8841c4438/extensions/cornerstone/src/ConnectedCornerstoneViewport.js#L79-L84

Another incosistent usage is in the react-cornerstone-viewport:
https://github.com/cornerstonejs/react-cornerstone-viewport/blob/7a669e715c47159821d419d3bb9d07bf1e43e742/src/CornerstoneViewport/CornerstoneViewport.js#L235-L240

ivan-aksamentov on 7 Oct 2019

👍1

@ivan-aksamentov, thank you for the detailed write-up! Internally, we're aware that entirely too much data is being jammed into Redux. We're actively monitoring new PRs to make sure we don't continue this creep, and we're working to simplify this portion of the code base.

A large part of the problem is that the bulk of these changes were made as part of a massive refactor to use React. It's common for new React developers to want to make _everything_ reactive. Now that we're here, our appetite for large refactors has waned, so we're trying to clean these things up as we touch relevant portions of the code base.

Viewports in particular are hairy because they're an "extension point", with each exposed viewport module potentially requiring different data in order to function. We're working to more clearly define the responsibilities of core, viewer and viewport extensions in this handshake. Currently, I believe we're leaning toward information like: ids, layout, etc. continuing to exist in Redux, while core would expose the remaining data (and DICOMWeb / API calls, cached image data, etc) through services, some of which we might expose to extensions.

The path's not super clear yet, but please feel free to weigh in or draft reports of other problems/solutions as you encounter them 🙏

dannyrb on 7 Oct 2019

👍1

@dannyrb
Hey Danny, I understand. We've monitored the refactoring efforts from meteor to react. I agree that a bit of cleanup is needed.
However, I would disagree that "too much data is being jammed into Redux". In fact, I would like to have more state and more actions to be available explicitly for 3rd parties in order to develop richer value-added products.

For example, I am currently figuring how to programmatically (without drag events) set series into viewport (in particular, SEG overlays) and how to programmatically jump to a slice in the viewport. I think these are the ideal candidates for Redux actions, however currently VIEWPORT::SET seems to be quite broken and, even if it wouldn't be, I just don't have enough data to fill this action anyways. I would like to have this data available in Redux state. I am going to add a couple of feature requests for these use-cases and would like to collaborate on the implementation.

ivan-aksamentov on 7 Oct 2019

I believe we have very similar goals then ^_^

There is an in-progress PR here that adds some of that functionality to react-cornerstone-viewport, or at least makes it a bit cleaner: https://github.com/cornerstonejs/react-cornerstone-viewport/pull/41

Where you can now set imageIdIndex for the component, and it will scrollToIndex. The API changes we made there are a large part of why I am now touching the VIEWPORT actions as I integrate the updates.

I'm torn on adding more data to Redux. It's a bit odd to add _everything_ we need for _every_ possible viewport implementation, overlay, etc. If we have key bits of information that should cause the view to change, like a studyId, then we should include that -- but then that studyId can be used to query a service for other data that doesn't need to be reactive.

I'm all ears here, as I'm new-ish to React development. If you have any specific problems that are not yet solved, or suggestions around a cleaner handshake, we could set up a quick call?

dannyrb on 8 Oct 2019

🚀1

@dannyrb https://github.com/OHIF/Viewers/pull/1224 resolves part of the mentioned problems. Consider adopting a stable action layout is a good idea, we can move forward refactoring the actions as devs touch in related code or we can create tasks to refactor each, this definitely improves consistency.