[WIP] Fix app state clash on browser refresh #1496
Conversation
When you're working with an app and you refresh the browser, the app shows as loading, then the content loads, then you see an empty card, and then you see the content again. In order to solve this issue, let's dig into the three ways in which an app is loaded: A. On first load 1. The app state reducer is called with a `null` state (you see an empty card) 2. The store is initialized (you see an empty card) 3. The app state reducer is called with a state loaded from the store (you see the actual content) B. Coming back from another application 1. The app state reducer is called with the cached previous state (you see the old content) 2. The app statee reducer is called with a state loaded from the store (you see the actual content) C. After refreshing the browser 1. The app state reducer is called with a `null` state (you see an empty card) 2. The app state reducer is called with the cached previous state (you see the old content) 3. The store is initialized (you see an empty card) 4. The app state reducer is called with a state loaded from the store (you see the actual content) The issue at hand is caused by step C.3, which shouldn't take place, since the store is already initialized. I believe C.3 to be a bug from AragonAPI, which @Schwartz10 already [reported](aragon/aragon.js#396). A downstream fix for this bug can only take place in the app state reducer, since it is called before the store. We need a way to detect that step C.3 is taking place and refrain from updating the state before it finishes loading, since the old state that we already have is more complete. But how to detect C.3 based solely on the state? Sure, you can detect that the store is being initialized, but how can you know that it's not loading for the first time? What I came up with was to store the previous state that the app state reducer received, and compare it to the current one. C.3 is the only case in which the previous state has elements that the current state doesn't. If C.3 is happening, don't update the state; just use the previous one. And as soon as the current state is at least as complete as the previous one, update it.
There was a problem hiding this comment.
Hey @e18r i don't have the time to fully test this by spinning up TPS, but i would probably tend to lean away from implementing something like this. Super cool idea, but when switching between apps there might be unexpected behavior because the app state reducer file goes through a new run-time. Using global state outside of the reducer can have some weird unintended side effects that might hard to foresee.
Maybe you planned for that in which case I would say just try every combination of possible load timing, app switching, and data fetching you can. But i'd be very careful!
Fixing aragon/aragon.js#396 is a more stable solution
There was a problem hiding this comment.
As @Schwartz10 said in his comment, app-state-reducers themselves are not stateful. This is a good idea, but it will probably generate some technical debt if moved into script.js, and depending on how the aragonSDK is updated to mitigate this issue, and will possibly lead to their changes breaking this, which would be painful for us to keep up on.
|
@topocount @Schwartz10 Thank you for your feedback. Do you consider it'd be better to leave issue #1411 open until aragon/aragon.js#396 gets fixed? |
|
|
||
| const appStateReducer = currentState => { | ||
| const state = getContentHolder('entries', currentState, prevState) | ||
| prevState = { ...state } |
There was a problem hiding this comment.
Why not just prevState = state? Does it need to be a brand new object?
There was a problem hiding this comment.
As long as the state doesn't get changed later on, it doesn't need to.
In principle, the practice is to create a newState object instead of modifying the state. But just to be safe, I did that. Cause if it gets changed, things can get nasty.
There was a problem hiding this comment.
I suspect that in general we return new state objects too frequently, and that this is part of what causes unwanted refreshes of our apps.
|
@e18r Is this PR still needed? Can we close it? |
|
@stellarmagnet I'm not completely sure we reached a consensus regarding what to do with this. A) Close this and wait for aragon/aragon.js#396 to get fixed, or Option A is the easiest, but the underlying issue could remain unsolved for a longer time; option B would require some thorough testing and QA to make sure this is a safe thing to do. |
stellarmagnet
changed the title
|
For me this PR seems quite relevant and a smart solution for an existing problem, independently of what is going to be the final solution from aragon upstream, we cannot rely on undetermined time for it to be fixed, but Emilio's current proposal seems to be solving this from today, so I would vote to merge that... The reason I didn't encourage this before, was because I was waiting to see if we can properly test the different scenarios that Jon pointed in his comment to ensure we are not breaking anything else, once we test that is working as expected, I would definitely merge this. Thoughts? |
Fixes #1411
When you're working with an app and you refresh the browser, the app shows as loading, then the content loads, then you see an empty card, and then you see the content again.
In order to solve this issue, let's dig into the three ways in which an app is loaded:
A. On first load
nullstate (you see an empty card)B. Coming back from another application
C. After refreshing the browser
nullstate (you see an empty card)The issue at hand is caused by step C.3, which shouldn't take place, since the store is already initialized. I believe C.3 to be a bug from AragonAPI, which @Schwartz10 already reported.
A downstream fix for this bug can only take place in the app state reducer, since it is called before the store. We need a way to detect that step C.3 is taking place and refrain from updating the state before it finishes loading, since the old state that we already have is more complete.
But how to detect C.3 based solely on the state? Sure, you can detect that the store is being initialized, but how can you know that it's not loading for the first time? What I came up with was to store the last state received by the app state reducer, and compare it to the current one. C.3 is the only case in which the previous state has content that the current state doesn't have.
If C.3 is happening, don't update the state; just use the previous one. And as soon as the current state is at least as complete as the previous one, update it.