Clean up measurements Redux states/actions #1881

joverlee521 · 2024-10-30T00:07:36Z

Description of proposed changes

Follow up to #1848, which left the measurements Redux state/actions in a bit of a mess as described in #1857.
This PR cleans up the mess to make upcoming changes for #1819 simpler 🤞

The main change is to fetch/load measurements JSON upfront instead of deferred loading with other sidecars JSONs. With this change, a lot of the Redux state complexities can be removed because the query params can be validated during the initial load of the page.

Related issue(s)

Resolves #1857

Checklist

Checks pass
If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
(to be done by a Nextstrain team member) Create preview PRs on downstream repositories.

Post-release

merge Update measurements loading auspice.us#107

Fetch and load the measurements sidecar JSON upfront in order to pave the way for simplifying the merging of controls and URL params. This will be especially useful once measurements data can add controls to the main tree, e.g. colorings. Subsequent commits will clean up the Redux state and actions now that we can assume that the measurements data have been loaded. Will also need to fix the bug of URL param `m_collection` no longer being respected on page load.

This is no longer required since we load the measurements JSON upfront and we can compare the URL param `m_collection` to the loaded collection.

Since the json values are cached for narratives, `parseMeasurementsJSON` should _not_ edit the json values directly. This commit fixes the TypeError that occurs when switching back to previously loaded dataset in narratives.

Allow the measurements panel to display any errors even if the measurements data failed to load. This will allow users to see errors in the measurement's panel even if the warning notification goes away.

Consolidate measurements controls and query param handling in `combineMeasurementsControlsAndQuery`. Now that we load measurements JSON upfront, we can use the collection's data to validate the query params on initial load of the page. Handling all measurements controls and query params in one place to make it easier to ensure they are kept in sync.

jameshadfield

I think this is a lot nicer to reason with than the previous version - thanks for refactoring. Thanks also for the testing narrative - I was just about to create one!

I'd think this PR is blocking until the auspice.us PR is ready.

jameshadfield · 2024-11-04T20:52:50Z

src/components/measurements/index.js

+      {measurementsError ?
+        <Flex style={{ height, width}} direction="column" justifyContent="center">
+          <p style={{ textAlign: "center" }}>
+            {measurementsError}
+          </p>
+        </Flex> :
+          (measurementsLoaded &&
+            <MeasurementsPlot
+              height={height}
+              width={width}
+              showLegend={showLegend}
+              setPanelTitle={setTitle}
+            />)


[for consideration]

The way this is written invites the question "what happens if there's no error but it's not loaded?". With the move to synchronous loading the state generated by loadMeasurements is either (loaded: true, error: undefined) or (loaded: false, error: warningMessage), but it's plausible the code will change later on and throw these two values out-of-sync. I'd suggest flipping the conditionals here: if loaded render the panel, if not show an error component with measurementsError || "unknown error".

Update: 6c2308a removes the entire panel if loaded: false, so these conditionals are never in play. We'll still get a warning notification "Failed to parse measurements collections" but no panel.

I'll leave the decision to you whether we display an error panel or just the notification and let you update the code accordingly.

I'd suggest flipping the conditionals here: if loaded render the panel, if not show an error component with measurementsError || "unknown error".

I'll go with this!

Update: 6c2308a removes the entire panel if loaded: false, so these conditionals are never in play. We'll still get a warning notification "Failed to parse measurements collections" but no panel.

It loads the page without the panel, but the user can still toggle on the panel via the sidebar controls. I think it's helpful to display the error message if the user decides to toggle on the panel after the warning notification has disappeared.

Updated in cab837e

jameshadfield · 2024-11-04T21:20:37Z

src/actions/recomputeReduxState.js

+  } else {
+    // Hide measurements panel if loading failed
+    controls.panelsToDisplay = controls.panelsToDisplay.filter((panel) => panel !== "measurements");
+    controls.canTogglePanelLayout = hasMultipleGridPanels(controls.panelsToDisplay);
+    controls.panelLayout = controls.canTogglePanelLayout ? controls.panelLayout : "full";


For completeness I'd also suggest removing any measurements queries. (There's no partial measurements state which is nice - it's either the loaded state or the default state.)

Ah, good catch! Will do.

Added in c03d336

src/actions/measurements.js

jameshadfield · 2024-11-04T21:41:46Z

src/actions/measurements.js

+  if (measurementsData === undefined) {
+    // eslint-disable-next-line no-console
+    console.debug("No measurements JSON fetched");


For a dataset without measurements measurementsData=false so we're getting the warning message when we shouldn't. This happens because while the dispatch object sets measurementsData: undefined that won't override the default arg value of createStateFromQueryOrJSONs({measurementsData = false, ...}). This is a feature / bug of JS depending on your point of view:

function test({aaa=false}={}) {console.log("inside", aaa)} test() // inside false test({aaa: undefined}) // inside false

You can use null if you want, but I'd suggest false

Ah good catch, I'll go with false.

I ended up just checking measurementsData is a falsy value in 0069e09.

jameshadfield · 2024-11-04T21:46:29Z

src/actions/measurements.js

@@ -170,7 +170,8 @@ const getCollectionDisplayControls = (controls, collection) => {
 };

 const parseMeasurementsJSON = (json) => {
-  const collections = json["collections"];
+  // Avoid editing the original json values since they are cached for narratives
+  const collections = cloneDeep(json["collections"]);


[aside]

When we're not viewing a narrative we could avoid this and reduce the memory footprint. I don't actually want to do it because it makes testing harder and will invariably result in bugs somewhere, but something that came to mind as I read this.

Yeah, I thought about adding some conditionals around narratives, but like you said, it seemed like that would eventually result in bugs somewhere 🙈

@jameshadfield

Switch the conditionals for rendering so that the Measurements panel always shows the appropriate content. This adds a default error message for the panel in case the `measurementsLoaded` and `measurementsError` states are ever out of sync. Based on feedback from @jameshadfield <#1881 (comment)>

@jameshadfield

Fix bug where the warning notification for measurements JSON was being fired even when the dataset did not request the measurements panel. Based on feedback from @jameshadfield <#1881 (comment)>

Updating to support upcoming changes in Auspice from <nextstrain/auspice#1881>

joverlee521 added 7 commits October 28, 2024 11:41

Remove excess whitespace

d9e679d

Remove unused LOAD_MEASUREMENTS action

047de26

Remove unused UPDATE_MEASUREMENTS_ERROR action

a5bc80f

Remove controls.measurementsCollectionKey Redux state

0f67361

This is no longer required since we load the measurements JSON upfront and we can compare the URL param `m_collection` to the loaded collection.

parseMeasurementsJSON: deep clone json.collections

0861269

Since the json values are cached for narratives, `parseMeasurementsJSON` should _not_ edit the json values directly. This commit fixes the TypeError that occurs when switching back to previously loaded dataset in narratives.

measurements: display error even if data not loaded

a2431ae

Allow the measurements panel to display any errors even if the measurements data failed to load. This will allow users to see errors in the measurement's panel even if the warning notification goes away.

joverlee521 requested a review from jameshadfield October 30, 2024 00:07

nextstrain-bot temporarily deployed to auspice-measurements-re-qyjyui October 30, 2024 00:07 Inactive

joverlee521 added the preview on nextstrain.org label Oct 30, 2024

nextstrain-bot mentioned this pull request Oct 30, 2024

[bot] [DO NOT MERGE] Test Auspice PR 1881 nextstrain/nextstrain.org#1057

Closed

joverlee521 added 2 commits October 29, 2024 17:24

Hide measurements panel if loading failed

6c2308a

joverlee521 force-pushed the measurements-redux branch from 30d864e to 6c2308a Compare October 30, 2024 00:24

joverlee521 temporarily deployed to auspice-measurements-re-qyjyui October 30, 2024 00:25 Inactive

jameshadfield requested changes Nov 4, 2024

View reviewed changes

joverlee521 added 3 commits November 5, 2024 11:40

loadMeasurements: fix bug for warning notifications

0069e09

Fix bug where the warning notification for measurements JSON was being fired even when the dataset did not request the measurements panel. Based on feedback from @jameshadfield <#1881 (comment)>

Remove measurements query params if loading failed

c03d336

joverlee521 added a commit to nextstrain/auspice.us that referenced this pull request Nov 5, 2024

Load measurements JSON with createStateFromQueryOrJSONs

4150c56

Updating to support upcoming changes in Auspice from <nextstrain/auspice#1881>

joverlee521 mentioned this pull request Nov 5, 2024

Update measurements loading nextstrain/auspice.us#107

Merged

1 task

jameshadfield approved these changes Nov 6, 2024

View reviewed changes

joverlee521 merged commit 540785d into master Nov 6, 2024
26 checks passed

joverlee521 deleted the measurements-redux branch November 6, 2024 19:05

joverlee521 added a commit to nextstrain/auspice.us that referenced this pull request Nov 6, 2024

Load measurements JSON with createStateFromQueryOrJSONs

0fa1d6e

Updating to support upcoming changes in Auspice from <nextstrain/auspice#1881>

joverlee521 added a commit to nextstrain/auspice.us that referenced this pull request Nov 6, 2024

Load measurements JSON with createStateFromQueryOrJSONs

418c03b

Updating to support upcoming changes in Auspice from <nextstrain/auspice#1881>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up measurements Redux states/actions #1881

Clean up measurements Redux states/actions #1881

joverlee521 commented Oct 30, 2024 •

edited

Loading

jameshadfield left a comment

jameshadfield Nov 4, 2024

joverlee521 Nov 5, 2024

joverlee521 Nov 5, 2024

jameshadfield Nov 4, 2024

joverlee521 Nov 5, 2024

joverlee521 Nov 5, 2024

jameshadfield Nov 4, 2024

joverlee521 Nov 5, 2024

joverlee521 Nov 5, 2024

jameshadfield Nov 4, 2024

joverlee521 Nov 5, 2024

Clean up measurements Redux states/actions #1881

Clean up measurements Redux states/actions #1881

Conversation

joverlee521 commented Oct 30, 2024 • edited Loading

Description of proposed changes

Related issue(s)

Checklist

Post-release

jameshadfield 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joverlee521 commented Oct 30, 2024 •

edited

Loading