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

View-Only: Fixes Redux State Corruption #245

Merged
merged 2 commits into from
Mar 29, 2020
Merged

Conversation

mattxwang
Copy link
Collaborator

PR Fix implemented by @jamieliu386:

The bug:

  1. Open a sketch in view-only mode.
  2. Go to the sketches page and try to download your most recently viewed sketch.
  3. The sketch title and extension are as expected, but the code is the code from the view-only sketch.

This happens because the Redux state is corrupted with mostRecentProgram; this is a temporary bandaid that properly deals with the Redux state, but doesn't solve the core problem.

This PR fixes the bug by saving the original code for the most recently viewed program when View-Only is entered, and then restores it before the component unmounts.

@jamieliu386 and/or @krashanoff, I'd like to see if there's a quicker bandaid style fix for this - refactoring mostRecentProgram and our Redux user manager (i.e. clearing out the state when the user logs out, storing the right things in cookies, etc.) is likely out of scope for the View Only PR, but I'm concerned about the dependence on the lifecycle hook.

jamieliu386 and others added 2 commits March 4, 2020 17:29
The bug:
Open a sketch in view-only mode. Go to the sketches page and try to download your most recently viewed sketch. The sketch title and extension are as expected, but the code is the code from the view-only sketch.

This fixes the bug by saving the original code for the most recently viewed program, and restoring it before the component unmounts.
@mattxwang
Copy link
Collaborator Author

Just added a check to make sure the user is logged in when we do the saving; if not, then it didn't work.

Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just got around to testing this out. The quick fix works. Will look into a more "bandaid style fix" for the root of the problem and report back.

@mattxwang
Copy link
Collaborator Author

Just got around to testing this out. The quick fix works. Will look into a more "bandaid style fix" for the root of the problem and report back.

Awesome, sounds good. If we can't find one super quickly, I don't mind merging this, then merging view only, then the go backend, and then reworking how mostRecentProgram and some of our Redux actions/state holders work.

Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over things a bit more, this is definitely something that needs to be handled in a separate PR. As it stands, view-only will be refactoring a great deal of our existing code in addition to adding a brand new feature. Though it may be a bit rash, I think that for the time being we will have to live with this quick fix and resolve it later down the line since it likely requires a host of other refactors.

@mattxwang
Copy link
Collaborator Author

Sounds good, thanks for the comment Leo. merging!

@mattxwang mattxwang merged commit 55cd132 into view-only Mar 29, 2020
@mattxwang mattxwang deleted the view-only-fix-dl branch March 29, 2020 03:08
@krashanoff krashanoff added the bug Something isn't working label May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants