feat: delay pending-in-queue banner #8309
Conversation
| return | ||
| } | ||
| setShowAlertPendingInQueue(true) | ||
| }, 5000) |
There was a problem hiding this comment.
How about using the difference here (now.subtract 5 sec) so that loading a new workspace doesn't reset the timer to 5 seconds?
There was a problem hiding this comment.
I'm not sure that I understand what you mean. Could you please elaborate a bit more about the case you have in mind?
There was a problem hiding this comment.
So let's say you:
- Start a workspace build
- Wait 3 seconds
- Reload the workspace page
- Build still pending, we start the timer with hard-coded 5 second wait
- Queue banner is shown (time since build started 8 seconds vs 5 seconds)
Does this make sense?
There was a problem hiding this comment.
Make sense now, thanks.
I used the diff, feel free to review that part.
|
|
||
| if ( | ||
| workspace.latest_build.status === "pending" && | ||
| workspace.latest_build.job.queue_size > 0 |
There was a problem hiding this comment.
I noticed we're not setting setShowAlertPendingInQueue(false) anywhere? Perhaps this condition could be inverted and placed at the top with an false + early return. Should simplify the other checks as well.
|
Forgot to mention in review, but I think this feature can still benefit from @spikecurtis suggestion to make it stickier, i.e. once shown, show for at least 250ms. There’s still the edge case of the job start coinciding with the 5 second delay which could result in flicker. How likely that is to happen will largely depend on the deployment. Some may often see the flicker whereas those not close to the 5s start range won’t. |
I tried to implement it, but as long as the React component is re-rendered when the workspace update is coming, it is hard to determine if the banner was already presented to start another timer. I'm open to ideas or contributions if somebody knows how to achieve this effect in the React, not hacky, way. |
I transformed this requirement to wait for 250ms before hiding the banner. It seems to be more clear, but I have a feeling that it is still a bit hacky. I will defer the judgment or final polishing to @BrunoQuaresma. |
| setTimeout(() => { | ||
| setShowAlertPendingInQueue(false) | ||
| }, 250) | ||
| return |
There was a problem hiding this comment.
I wonder if we need to return a clearTimeout here too? Otherwise navigating away from this view could trigger the timer after-the-fact (is that a problem?). Then again, if the function is triggered every time useEffect has new values, we need to make sure we don't repeatedly set/clear the timeout when e.g. new workspace data comes in (let's say there are 10 updates <250ms apart each, then this would ultimately take ~2.5s to trigger).
There was a problem hiding this comment.
I played a bit, and didn't observe any unexpected side effects of having clearTimeout, so added it 👍
There was a problem hiding this comment.
Changes look good! I'm also no React expert, so deferring final verdict to @BrunoQuaresma in case there's a cleaner way to do this.
Fixes: #8254
This PR adds the logic to show the pending-in-queue banner if the build is parked for more than 5 seconds. The idea is to reduce the blink effect if the workspace build is being processed after 1-2 sec after creation.
Notice: I'm not TypeScript/JS magician, so I apologize if I used the wrong concepts.