website patch for SelectionStateInfo changes

[interim17]

Time estimate or Size
small

Problem

Viewer release v3.8.4 includes breaking changes to SelectionStateInfo from this PR #400.
Patching advances #510 and #511

Solution

This patch:

• installs the newly released viewer
• removes individual colorChange from redux as it is no longer part of SelectionStateInfo
• updates the selector getSelectionStateInfoForViewer to instead use the current UIDisplayData as the value for appliedColors

Steps to Verify:

1. Try changing some colors to make color picker is not broken!
2. Verify that side panel and rendered scene are in sync.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟡	Statements	66.7% (+0.03% 🔼)	663/994
🟡	Branches	66.21%	96/145
🔴	Functions	35.71% (-0.11% 🔻)	90/252
🟡	Lines	65.05% (+0.14% 🔼)	590/907

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	... / basic.ts	80.95% (-4.76% 🔻)	100%	42.86% (-14.29% 🔻)	81.82%

Test suite run success

121 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from c4a433b

[frasercl]

Seeing this ColorChange import get removed here prompted me to go back and check on its fate at the simularium-viewer level, since I recall getting the impression that simularium/simularium-viewer#400 made this type obsolete. A couple observations:

• This type is still defined and exported in simularium-viewer, but is no longer used anywhere in the package. Could it be removed entirely in a future PR?
• There's a fair bit of ColorChange-related Redux plumbing in this repo which is currently untouched by this PR (an action type and action creator in src/state/selection, and a chain of components that depend on them). Can that stuff come out too?

[frasercl]

Following on from previous comment: I think I've chased the path of SetColorChangeAction far enough to understand what's going on with the remaining Redux stuff here.

With this arm of the reducer gone, SetColorChangeAction no longer has any effect on state via "traditional" redux means. But it's still picked up by a logic, which dispatches a new ReceiveAction to make the necessary changes on the trajectory branch. As I understand it from a bit of searching, logics are primarily a tool for adding async behavior or side effects to Redux. But it looks like the purpose of that logic is to let this action from the selection branch select and modify state from the trajectory branch.

A couple suggestions to investigate based on this:

• Sounds like you could still easily redo the type of SetColorChangeAction a little bit to no longer depend on the ColorChange type out of simularium-viewer, especially since that type is no longer included in Redux state on this side.
• If the only purpose of SetColorChangeAction is now to dispatch another action on the trajectory branch, could it be refactored as an action on trajectory itself? The code currently in state/selection/logics.ts could be reworked into an arm of the trajectory reducer.
• In fact, that logic appears to be the only place the receiveAgentNamesAndStates action creator is used?

[interim17]

Love these comments and appreciate your review.

To start, you're absolutely right that the ColorChange type no longer has a role in the sim-viewer repo and can be removed there entirely. I think it's still useful here, will re-declare locally.

I tried to write this patch with as few lines as possible, and may may have introduced some confusion, my long term thinking is in this PR.

Yes, the SetColorChangeAction plumbing on the selectionbranch has been orphaned and only exists to trigger updates in a different state branch. You are correct. Two things are going to happen in the forthcoming change set that complicate/justify this (imo).

• This selection branch action/logic is going to start managing putting color settings into browser storage, which are not part of the Redux store per se, but are persistent storage of user selections and seem more fitting in this branch than trajectory.
• The agentUiNames (and its action receiveAgentNamesAndStates)is going to be renamed defaultUIData and we are adding userSelectedUIData. Until now this data lived in the trajectory branch as it is derived by parsing the trajectory and we could keep it there, but move the userSelectedUIData to selection branch since it represents user choices. This would give the plumbing in question relevance.
  But for now I haven't. I kept the two together in the trajectory branch alongside the enum toggle that indicates which one of the two is currently applied (currentColorSettings) because in practice the current state of the application has a lot to do with how those three things relate and I felt that keeping them together made that apparent/readable.

tl;dr I'll pull the old viewer code, and we could move everything over to trajectory and say, "who cares that the trajectory branch is now managing browser storage and things that look like user selections". Or we can maintain this somewhat confusing weaving between the two state branches. Or a secret third thing. That was long, lmk what you think.

[frasercl]

I'm not all that familiar with the logic behind the branch breakdown of Redux state in this app... it sounds like part of the problem here is that user-selectable colors don't have an obvious home in either branch? If trajectory is for static info loaded from the network and selection is for the user's modifications, then they belong in selection. But if trajectory is for describing agents and selection is about the single agent the user is currently focused on, they belong in trajectory. Sounds like @meganrm might have a clearer plan on this, but my vague thoughts are:

• I like the idea above that (if I'm reading right) trajectory gets "default values" from the trajectory and selection gets user overrides. Fits with the framework better as I understand it, plus over on the volume viewer side we're in the process of learning that it's good to have a clear spot for default values to live.
• If you're in the market for a secret third thing, and if I'm understanding right that color selections aren't quite at home in the current branches, don't forget to consider: make another branch where they are?

Ultimately, I'm less worried about organization than I am about having the Redux plumbing make sense. My main concern about where this PR leaves things was that SetColorChangeAction basically becomes an alias for an entirely different action via a logic - meaning that a. the reducer runs twice and does nothing the first time, b. it's not obvious what the action actually does, since logics (to me) imply side effects that happen alongside the main state change the action makes. (Updating browser storage sounds like a great case for a logic to me.)

If this is building towards a more settled state in a future PR - and it sounds like it is - I'm okay merging.

[interim17]

Yes to: building towards more settled state.
Yes to: these things will belong in selection primarily.
To settle in future PR: how best to use and name the Redux plumbing when using it to move data around, but the data lives outside the store (browser storage).

Thank you for review!

[interim17]

@frasercl Checked in with @meganrm and discussed state branch breakdown. Her suggestion for the next PR is to actually move two pieces from trajectory back over to selection... so, selection branch plumbing is going to stick around in some form for sure.