- Notifications
You must be signed in to change notification settings - Fork1
fix: update&start outdated workspaces#128
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
URI handling was not able to start workspaces that were stopped and also outdated.
During URI handling we can wait for workspaces to start up. The existing implementationhad no visual feedback that we are waiting/doing anything. With this PR the header bar showsa nice work in progress visual. Additionally, the deployment URL was not properly refreshedwhen switching between different urls via URI handling. This is also fixed by forcing TBXto render a blank page for a very short period of time and then going back to the main page.
| */ | ||
| suspend fun refreshMainPage() { | ||
| ui.showUiPage(CoderPage.emptyPage(this)) | ||
| delay(10.milliseconds) |
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.
Could you add a comment for why we need to do a delay here for the uneducated? 😄
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.
Thank you for the feedback, I've addressed it in the code, but here is a summary:
From testing and decompiling the Toolbox code, it seems that TBX uses an internal shared flow with a buffer of 4 items and a DROP_OLDEST strategy. Both showUiPage and showPluginEnvironmentsPage send events to this flow. If we emit two events back-to-back, the first one often gets dropped and only the second is shown. To reduce this risk, we add a small delay to let the UI coroutine process the first event. Simply yielding the coroutine isn't reliable, especially right after Toolbox starts via URI handling. Based on my testing, a 5–10 ms delay is enough to ensure the blank page is processed, while still short enough to be invisible to users.
It goes without saying that the API is limiting and we don't have the callbacks or the hooks necessary to observer when a page goes active. That would be much cleaner.
In fact the whole thing is necessary because the url/title on the main page is only refreshed if we're navigating to the main env page from another page. If TBX is already on the main page the title is not refreshed hence we force a navigation from a blank page. We've raised a ticket in the past, once that is fixed the blank page workaround will no longer be needed and as a consequence the delay will be dropped as well.
Before agent matching we check the state of a workspace and if it is stoppedwe automatically start it (if Disable autostart is not enabled). However theagent matching code still received an old copy of the stopped workspace. Stoppedworkspaces don't have resources.
URI handling was not able to start workspaces that were stopped and also outdated. With this PR we check the ws status and call the proper REST endpoint depending on whether the workspace is outdated or not.
During URI handling we can wait for workspaces to start up. The existing implementation had no visual feedback that we are waiting/doing anything. With this PR the header bar shows a nice work in progress visual. Additionally, the deployment URL was not properly refreshed when switching between different urls via URI handling. This is also fixed by forcing TBX to render a blank page for a very short period of time and then going back to the main page.