search

chanzuckerberg / galago

Interpretation aids for genomic epidemiology
https://chanzuckerberg.github.io/galago/
MIT License
8 stars 3 forks source link

[Bug] `state` gets confused when user (1) loads demo (2) back button (3) uploads data #157

Open sidneymbell opened 2 years ago

sidneymbell commented 2 years ago

@happyimadesignr -- can you please provide the steps to reproduce?

image
happyimadesignr commented 2 years ago

Hi, sure

Steps to reproduce bug:

  1. Click on demo button and view demo page
  2. Click back button in browser ui, so I can upload my own data
  3. Upload json and metadata file and click submit
  4. I am brought to the report page, but it already has data selected and report is already showing

If I click browser back button and click browser refresh button and reupload, it brings me to the starting page as expected.

sidneymbell commented 2 years ago

Thank you!!

sidneymbell commented 2 years ago

Need to fire the reset to defaults reducer whenever we arrive at the app page or try to load new data

vincent-czi commented 2 years ago

So I see two different ways to approach this:

  1. Whenever the user gets to the landing page / homepage, we reset the entire redux state so we start from a clean slate. Go back to defaultState via kicking off a reset to defaults action upon initial render of the LandingPage component.
  2. Leave state alone on LandingPage, and only reset to defaultState upon the user kicking off a (potentially) new tree interaction by either clicking "Load Demo" button or uploading a tree via "Select Tree JSON".

Let's discuss pros and cons:

(1) is definitely the easier option to implement. We just need a useEffect that fires once (via an empty dependency array) on render of LandingPage and kicks off the reset to defaults action. This clears the ground for the future interaction and everything should be fine if they do a new "Load Demo" or upload their own tree JSON. The issue with (1) is that it prevents the user from being on /app looking at a tree, hitting the back button (perhaps by accident), realizing they want to go back to their tree as they were viewing it before, and then clicking the forward button. As the app currently works, this does exactly what you'd want. You're looking at a tree with some set of filters, you go back to landing page, then hit forward and you're back to the same tree with same set of filters on it. The back/forward interaction does not impact redux state in and of itself, so your place on the tree is effectively "saved" as you navigate. If we don't really care about that, then we should definitely do (1).

(2) allows the back/forward "saving your place" behavior. If we do (2), it goes back but doesn't wipe out to defaults until you commit to the fact that you want a new tree, at which point it makes sense to wipe out to defaults as a preparatory step before loading in new tree info. The downside is that it's a bit more cumbersome in code: the places where we load up a tree for the first time (load demo, tree file uploaded) need to have the first part of their reducer's return statement do a reset to defaultState. (Huh, looks like load demo actually already does this, its top line is ...defaultState.) It's easy to implement, but couples the tree loading to the state reset very strongly: if we reorganized how loading data works later on, this could be a pain to de-couple.

So really it comes down to how much we want the ability to hit back button, then forward button, but not have lost the tree we were just on. If we don't care much about that, (1) is definitely the way to go. If we care strongly about that, then (2) is what we should do. If we're on the fence, it depends on how happy we are about coupling tree load to a state reset.

sidneymbell commented 2 years ago

Thanks for the clear explanation of your thought process and pros/cons, @vincent-czi . I always super appreciate this about working with / learning from you!

My initial thought is that the forward / backward buttons are very fragile as a way to interact with and control state of the application. I'd like to both discourage the user from coming to think this is something they can rely on and prevent them from accidentally shooting themselves in the foot and losing their data and progress if they refresh/leave.

What do you think about implementing (1) as a first step, and filing a separate issue to fire off a little dialog pop-up that says "are you sure you want to leave this page? state will be lost" when the user tries to hit the fwd/back/refresh/other url?