-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(app): store run step completion in redux #16570
Conversation
Add a redux substore for the setup step completion status that we'll use in place of react state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Agree with the overall approach!
@@ -68,4 +71,5 @@ export const rootReducer: Reducer<State, Action> = combineReducers({ | |||
sessions: sessionReducer, | |||
calibration: calibrationReducer, | |||
protocolStorage: protocolStorageReducer, | |||
protocolRuns: protocolRunReducer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm very excited about this!
), | ||
}) | ||
) | ||
}, [runId, protocolAnalysis, dispatch]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling this could cause an infinite re-render, but I could be wrong. We probably want to do something like protocolAnalysis.key
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup this happened instantly. This can be either a robot-side or app-side analysis so we have to try the id too but it works when you do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want some sort of store update here if a run is already in progress, and we don't actually need any setup, rather than forgoing the top level setup
field?
protocol analyses are absolutely not safe to use as hook dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
[It turns out that since we implemented the green checks displayed for completed steps as react component state, if you navigate away from the component it loses the state.
To fix this we can throw it in redux. This PR does that, in the app anyway.
testing
reviews
Closes RQA-3304