Skip to content

Improve consistency in client-side login/logout experience #590

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Improve consistency in client-side login/logout experience #590

wants to merge 4 commits into from
+130 −82

Conversation

EhabY
Copy link
Collaborator

@EhabY EhabY commented Sep 23, 2025

Fixes #498

@EhabY EhabY mentioned this pull request Sep 23, 2025
Closed
EhabY
EhabY commented Sep 23, 2025
View reviewed changes
@mtojek mtojek requested a review from mafredri September 23, 2025 13:26
EhabY
EhabY commented Sep 23, 2025
View reviewed changes
src/commands.ts
Comment on lines +292 to 298
if (!value) {
return null;
}
client.setSessionToken(value);
try {
user = await client.getAuthenticatedUser();
Copy link
Collaborator Author

@EhabY EhabY Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should we attempt to get the authenticated user when the token is a blank string (like when this first opens)?

@mafredri
Copy link
Member

I tried out this PR and while I see a "you've been logged in" popup in multiple windows, the other windows are still stuck on this screen and there is no attempt to re-connect to the workspace:

image

I imagine this dialogue should be dismissed and a connection re-attempted? The user experience of selecting cancel is pretty bad (closes the workspace and gives you an empty window). I did not try what happens if I select "login".

@EhabY
Copy link
Collaborator Author

EhabY commented Sep 24, 2025

I tried out this PR and while I see a "you've been logged in" popup in multiple windows, the other windows are still stuck on this screen and there is no attempt to re-connect to the workspace

Can you provide a reproducible for this? I am not sure what's special about some windows that they do not respond to the change 🤔

@mafredri
Copy link
Member

Can you provide a reproducible for this? I am not sure what's special about some windows that they do not respond to the change 🤔

@EhabY Sure, I'll write down what I did.

Open two VS Code windows to the same workspace (different folders in the same workspace, whilst logged in):

coder open vscode my-workspace.dev /home/coder/foo

and

coder open vscode my-workspace.dev /home/coder/bar

Now you should have two windows that are both. Make sure VS Code is set to restore previous windows after quitting.

Then:

  1. Quit VS Code
  2. Delete ~/Library/Application Support/Code/.../session (it's in globalStorage, not at the machine right now so I can't give you full path)
  3. Open VS Code
  4. Both windows should show the ^aforementioned dialogue
  5. Perform log in on one of the windows
  6. Notice the dialogue does not disappear on the second

Does this help?

@EhabY EhabY force-pushed the fix-login-logout-in-multiple-windows branch from 9b0ba3e to 05160e8 Compare September 25, 2025 09:09
@EhabY
Copy link
Collaborator Author

EhabY commented Sep 25, 2025 •
edited
Loading

Yes, thanks for the detailed reproducer! I can see the issue now: VS Code modals aren't dismissible without user action, which is by design (see microsoft/vscode#2732). I can adjust our flow so we don't close the window if the user signs in in the meantime (whether they click cancel, login or x). The logs in the other window show that the sign-in succeeded, so the real problem is our fallback when a login completes after the modal appears.

Do keep in mind that we cannot change the content of the modal once it's shown, so we either go for something less intrusive or just handle the fallback correctly.

Using the status bar for messages which need to be updated / dismissed is suggested there as the recommended approach in the issue I mentioned. What do you think?

@EhabY EhabY force-pushed the fix-login-logout-in-multiple-windows branch from 05160e8 to c7df45f Compare September 25, 2025 13:01
@mafredri
Copy link
Member

Oh, that's unfortunate, but understandable (that the modals can't be dismissed).

Using the status bar for messages which need to be updated / dismissed is suggested there as the recommended approach in the issue I mentioned. What do you think?

It's one option but I don't think it's a good one. It's fairly hidden for something that's required for your editor to start working.

I think updating the modal is the way forward. It could say something like "If you already performed login in another window, please use the cancel button."

image

This is what it looks like, after logging in from another window. See how it says "Opening remote" at the bottom, but nothing happens? I think it would be nice if we could make it successfully load the workspace in the background. Without looking at the actual code, perhaps we could do something along the lines of await Promise.race([loginmodal, loginevent]). If the workspace has loaded in the background, that's further confirmation for the user they can just hit cancel. Wdyt?

@EhabY
Copy link
Collaborator Author

EhabY commented Sep 26, 2025

I like this idea. Updating the modal to clearly indicate that a login happens in another window, plus adding background login capability, would be a solid improvement. I think this is possible but I'll be careful to avoid any racey conditions.

I think the following makes sense:

Description:

You must log in to access ${workspaceName}. If you already performed login in another window, please close this popup.

Then it's a race between two conditions as you said, 1. Login event, 2. Modal interaction. In the case of a login event, if the user clicks anything (x, Cancel, Login) it will just dismiss the modal and show the already connected workspace. The flow for the modal interaction stays the same as before.

@EhabY EhabY force-pushed the fix-login-logout-in-multiple-windows branch 2 times, most recently from 53432cb to 569a462 Compare September 27, 2025 08:02
EhabY
EhabY commented Sep 27, 2025
View reviewed changes
src/remote.ts Outdated
Comment on lines 244 to 252
// Try to detect any login event that might happen after we read the current configs
this.createLoginDetectionPromise();
// Get the URL and token belonging to this host.
const { url: baseUrlRaw, token } = await this.cliManager.readConfig(
parts.label,
);

// It could be that the cli config was deleted. If so, ask for the url.
if (
!baseUrlRaw ||
(!token && needToken(vscode.workspace.getConfiguration()))
) {
const result = await this.vscodeProposed.window.showInformationMessage(
"You are not logged in...",
const showLoginDialog = async (message: string) => {
const dialogPromise = this.vscodeProposed.window.showInformationMessage(
Copy link
Collaborator Author

@EhabY EhabY Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I could move the createLoginDetectionPromise into the showLoginDialog but we have a window of opportunity between creating the promise and the user ALREADY logging in. Not sure if this could ever happen

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved there for now so we do not forget to set the promise before potentially resolving

@EhabY EhabY force-pushed the fix-login-logout-in-multiple-windows branch from 67af845 to 4def400 Compare September 29, 2025 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mafredri mafredri Awaiting requested review from mafredri

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve consistency in client-side logout experience
2 participants
@EhabY @mafredri