Skip to content

Clear workspaces on reconnect and fetch immediately on return #182

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

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

code-asher
Copy link
Member

My thinking here is that if you try connecting to a new URL you still see the old workspaces until the new connection is successful which could be confusing (the new URL shows in the text field yet the workspaces below it belong to something else and the list will no longer update since the poller was stopped).

Also thought it might make sense to fetch immediately if you come back to this step since it is not clear that the data there is possibly stale (especially if you were on a previous/next step for a long time); I think I would assume a fresh fetch when I interact with the next/previous buttons but this is just my personal assumption.

Fetching immediately also "fixes" an interesting bug where if you hit back you are unable to connect to a workspace until the next poll goes through. I considered looking more into this but figured it would be best to block the next step on fresh data anyway.

@code-asher code-asher requested a review from fioan89 March 1, 2023 22:50
@fioan89
Copy link
Collaborator

fioan89 commented Mar 2, 2023

Yeah, the back issue was there for quite some time, and it was caused simply because the callback for enable/disable was executing only when the table selection changed. When you come back from IDE&Project view, the table is not yet changed, the old data is still present, with the same old selection. At some point the poller is updating the table values, which in the end triggers the callback that checks what is selected. Of course, cleaning up the elements in the table also triggers the callback...fixing the issue.

@@ -454,7 +456,7 @@ class CoderWorkspacesStepView(val enableNextButtonCallback: (Boolean) -> Unit) :

this.indicator.fraction = 1.0
updateWorkspaceActions()
triggerWorkspacePolling()
triggerWorkspacePolling(false)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see the harm in requesting the workspaces a bit earlier.

@code-asher
Copy link
Member Author

Ahhh I see, that makes sense. Thanks for explaining the issue!

My thinking here is that if you try connecting to a new URL you still
see the old workspaces until the new connection is successful which
could be confusing (the new URL shows in the text field yet the
workspaces below it belong to something else and the list will no longer
update since the poller was stopped).

Also thought it might make sense to fetch immediately if you come back
to this step since it is not clear that the data there is possibly
stale (especially if you were on a previous/next step for a long time);
I think I would assume a fresh fetch when I interact with the
next/previous buttons but this is just my personal assumption.

Fetching immediately also "fixes" an interesting bug where if you hit
back you are unable to connect to a workspace until the next poll goes
through.  I considered looking more into this but figured it would be
best to block the next step on fresh data anyway.
@code-asher
Copy link
Member Author

Going to hold off merging because I just rebased and now when I hit back the button never becomes available.

@code-asher code-asher force-pushed the asher/clear-fetch-workspaces branch from 3143ebb to 2b7b2a9 Compare March 2, 2023 20:24
@code-asher
Copy link
Member Author

code-asher commented Mar 2, 2023

I figured the easiest fix is to clear in onInit so I did that. It also unselects what was selected which...partly I think that makes sense, partly I am not sure.

Fixes the back button never being available.  Plus it makes it so you
have to wait for the next fetch which I think makes sense so you are not
trying to connect with potentially stale data.
@code-asher code-asher force-pushed the asher/clear-fetch-workspaces branch from 2b7b2a9 to e209bd3 Compare March 2, 2023 20:29
@code-asher code-asher merged commit 288a8a4 into main Mar 3, 2023
@code-asher code-asher deleted the asher/clear-fetch-workspaces branch March 3, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants