Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sidneymbell opened this issue Jul 27, 2022 · 5 comments
Assignees
Labels
Blocker for launch [type] bug Something isn't working

Comments

@sidneymbell
Copy link
Collaborator

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

image

@sidneymbell sidneymbell added the [type] bug Something isn't working label Jul 27, 2022
@happyimadesignr
Copy link

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
Copy link
Collaborator Author

Thank you!!

@sidneymbell sidneymbell added this to the Stable beta launch milestone Aug 2, 2022
@sidneymbell sidneymbell changed the title [Bug] Might be holding on to demo state? [Bug] state gets confused when user (1) loads demo (2) back button (3) uploads data Aug 2, 2022
@sidneymbell
Copy link
Collaborator Author

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

@vincent-czi
Copy link
Collaborator

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
Copy link
Collaborator Author

sidneymbell commented Sep 13, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocker for launch [type] bug Something isn't working
Projects
Status: Next up
Development

No branches or pull requests

3 participants