Stream execution progress to the action page #6565
Conversation
bduffany
force-pushed
the
exec-progress-web
branch
3 times, most recently
from
38a5e6f
to
35223bd
Compare
bduffany
changed the title
bduffany
changed the title
bduffany
requested review from
jdhollen and
siggisim
and removed request for
jdhollen
bduffany
force-pushed
the
exec-progress-web
branch
from
35223bd
to
dd8c700
Compare
|
One extra thing--might be worth putting a tiny little spinner next to the status or something (until it's completed, obviously)? As it is it's not obvious that the text will potentially update. I don't particularly like using a spinner, just think there should be something. |
app/invocation/execution_status.tsx
Outdated
| case ExecutionStage.QUEUED: | ||
| return "Queued"; | ||
| case ExecutionStage.EXECUTING: | ||
| // Return fine-grained progress using BB-specific enum |
There was a problem hiding this comment.
nit: for readability, might be better to just break out the executionState-based text into its own function and call it directly here--the fall-through is a bit awkward.
app/invocation/invocation.tsx
Outdated
| @@ -197,6 +197,12 @@ export default class InvocationComponent extends React.Component<Props, State> { | |||
| .catch((error: any) => { | |||
| console.error(error); | |||
| this.setState({ error: BuildBuddyError.parse(error) }); | |||
|
|
|||
| // To make manual testing easier, support a special 'retry' param that | |||
| // auto-reloads the page if there is an error. | |||
There was a problem hiding this comment.
why this one rpc + why this particular behavior?
There was a problem hiding this comment.
eh I'm not sure how useful this will be in general so I just reverted this for now. The commit is b6a0e9c in case we need it in the future
| @@ -74,6 +76,12 @@ export default class ExecutionCardComponent extends React.Component<Props, State | |||
| this.fetchUpdatedProgress(); | |||
| } | |||
|
|
|||
| // To help with manual testing, support a special "openFirst" URL param | |||
| // which automatically navigates to the first execution that we fetched. | |||
There was a problem hiding this comment.
Probably need to add a list of these somewhere (whether by an enum in code or just a list in a markdown file somewhere)--the way that they're placed right now means me and you will be the only people that know about them.
There was a problem hiding this comment.
(Reverted for now, same as above)
I agree, I was thinking of maybe a pulsing blue dot ("progress-blue" color) but something tells me it's going to look a little weird. Will play around with this in a separate PR |
Starting with the action page mostly as a proof of concept because it's easier to test than workflows and is maybe somewhat useful for larger actions. It will be more interesting to show this info on the queued invocation page (for workflow / remote bazel invocations).
Screen recording of an execution with an artificial 8s delay at the beginning of ExecuteTaskAndStreamResults + 750ms delay when transitioning between each execution state (otherwise the updates go by too quickly...) (also the "Unknown" execution status should probably be "Queued", need to fix that separately)
rec-2024-05-13_18.45.51.mp4
Related issues: N/A