feat(api, app): add state change information to rpc #5512
Conversation
This adds an extra structure of StateInfo to the RPC session that transmits information like the reason behind a state change to the app. As a first pass at using it, it also displays the message from a protocol pause in the pause alert box.
Codecov Report
@@ Coverage Diff @@
## edge #5512 +/- ##
==========================================
+ Coverage 65.83% 65.95% +0.11%
==========================================
Files 1094 1114 +20
Lines 31126 31358 +232
==========================================
+ Hits 20493 20681 +188
- Misses 10633 10677 +44
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Got some naming and type definition nits, but mostly I'd like to make sure there are unit tests in place for:
- RPC full session comes into api-client with
stateInfoundefined (old robot) - RPC full session comes into api-client with
stateInfo === {}(initial state) - RPC full session comes into api-client with
stateInfoset with properties - Same tests for RPC state update notification
The RPC API client is not typechecked because it's old and janky, so these unit tests are (even more) important to have in place when we make changes
| self.state = None | ||
| self.state: 'State' = None | ||
| #: The current state | ||
| self.stateInfo: 'StateInfo' = {} |
There was a problem hiding this comment.
See comment about statusInfo vs stateInfo. Also, is this only ever going to be used for pause? Should it be pauseInfo?
There was a problem hiding this comment.
It can be used for any state; many of the functions will only be used in pause for now, but e.g. changedAt and estimatedDuration could be used for more.
| if state not in VALID_STATES: | ||
| raise ValueError( | ||
| 'Invalid state: {0}. Valid states are: {1}' | ||
| .format(state, VALID_STATES)) | ||
| self.state = state | ||
| if user_message: |
There was a problem hiding this comment.
Wouldn't it be easier to just create a new self.stateInfo object here? No need to clear previous values, just overwrite instance with a new one.
There was a problem hiding this comment.
It would definitely be cleaner if I wanted to have all the keys there all the time but I kind of like the idea of removing keys and making sure they always have values if present
There was a problem hiding this comment.
Have it your way, but I'll make one last statement. Up to you. It's not that big a deal.
So... I am not a fan of having several if / else clauses that do almost exactly the same thing. You can filter out the None values when creating payload below.
| if state not in VALID_STATES: | ||
| raise ValueError( | ||
| 'Invalid state: {0}. Valid states are: {1}' | ||
| .format(state, VALID_STATES)) | ||
| self.state = state | ||
| if user_message: |
There was a problem hiding this comment.
Have it your way, but I'll make one last statement. Up to you. It's not that big a deal.
So... I am not a fan of having several if / else clauses that do almost exactly the same thing. You can filter out the None values when creating payload below.
| `Run paused${buildPauseMessage(message)}` | ||
|
|
||
| const buildPauseUserMessage = (message: ?string) => | ||
| message && <div className={styles.pause_user_message}>{message}</div> |
There was a problem hiding this comment.
<p> is more appropriate for copy than a <div>
This adds an extra structure of StateInfo to the RPC session that
transmits information like the reason behind a state change to the app.
As a first pass at using it, it also displays the message from a
protocol pause in the pause alert box.
Testing
protocol_context.pause()(orrobot.pause()in your protocol the message shows up in the alert boxThis is a precursor and set of new functionality for adding the rest of the window switch pausing; displaying the user pause message is just a good example of what it can do.