fix(protocol-designer): retain timeline data when overwriting protocol via import #17476
Conversation
…l via import closes AUTH-1429
|
closing this for now because this doesn't actually fix the bug i described. i think this may have always been in PD because if you import the same protocol over and over, the logic in timeline middleware that determines whether or not we need to recompute the timeline will not get triggered since there is no difference in application state (since the protocols are identical). i still think the reordering i have is more correct but i don't think this change is necessary |
|
okay, i reopened because i think this is what we actually want now:
|
| @@ -63,10 +66,13 @@ export const makeTimelineMiddleware: () => Middleware<BaseState, any> = () => { | |||
| const needsRecompute = hasChanged(nextSelectorResults, prevTimelineArgs) | |||
| // update memoized values | |||
| prevTimelineArgs = nextSelectorResults | |||
| return needsRecompute | |||
| return needsRecompute || actionType === 'LOAD_FILE' | |||
There was a problem hiding this comment.
Why do we need to explicitly recompute when you load a file now? previously, did hasChanged() return true when you upload a file?
There was a problem hiding this comment.
oh wait, seeing your comments, i guess this was always broken??
There was a problem hiding this comment.
it returns true when you upload a new file. this is because the protocol actually contains different information. if you upload the same exact protocol the timeline does not get recomputed. the problem though is that the load file action wipes out timeline data, so you're left with a step forms but no timeline
There was a problem hiding this comment.
its crazy to me that we never noticed this before. i guess its not common for someone to reimport the same protocol that is currently loaded
Overview
This PR moves redux-thunk to get applied to actions AFTER timeline middleware so that if timeline middleware ever depends on a thunk action our logic will still work as expected. This PR also explicitly regenerates the timeline and substeps whenever a protocol is imported into PD.
This fixes a bug where importing a protocol (when you already have a protocol loaded into application state) does not trigger timeline computation, which means that sub steps, liquid tracking, and commands never get loaded into application state.
closes AUTH-1429
Test Plan and Hands on Testing
Import a protocol into PD (like the one attached to this PR), import it again, and make sure that exported protocol you get back out has liquid handling commands. Also make sure the starting deck state and ending deck state are different.
Reagent-transfer-10.json
Changelog
Review requests
See test plan
Risk assessment
Medium/high