-
Notifications
You must be signed in to change notification settings - Fork 887
feat: delay pending-in-queue banner #8309
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
Conversation
return | ||
} | ||
setShowAlertPendingInQueue(true) | ||
}, 5000) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Fixed
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.