Skip to content
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

Change CodyAuthenticationManager from project to application level service #2190

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

pkukielka
Copy link
Contributor

Changes

After discussion with @danielmarquespt we decided account settings should be global, shared across all IntelliJ projects.

To reflect that fact I'm changing CodyAuthenticationManager to be APP level service instead of project level.
Notifications about account state change are pushed to all open projects.
All projects use shared storage, so changes done when project is closed are also properly loaded upon opening that project.

All relevant changes are in the CodyAuthenticationManager, all the rest is removal of unneeded project parameter.

Test plan

Scenario 1

  1. Sign-in with any account
  2. Restart the IntelliJ
  3. Make sure you are still logged in

Scenario 2

  1. Sign-in with any account, add a few more
  2. Restart the IntelliJ
  3. Make sure you are still logged in, with the same account

Scenario 3

  1. Sign-in with any account, add a few more
  2. Switch account
  3. Restart the IntelliJ
  4. Make sure you are still logged in, with the same account you switched to

Scenario 4

  1. Sign-in with any account, add a few more
  2. Sign-out
  3. Restart the IntelliJ
  4. Make sure you are not logged in

Scenario 5

  1. Sign-in with any account, add a few more
  2. Switch account
  3. Open a second IntelliJ project
  4. Make sure in the second IntelliJ window you are logged in, with the same account you switched to

Scenario 6

  1. Open two IntelliJ projects side by side
  2. Open Account panel in both of them
  3. Make sure that switching account in one of them causes switch to the same account on the second

Scenario 7

  1. Open two IntelliJ projects side by side
  2. Open Account panel in both of them and make sure they have active the same account
  3. Close one of the instances
  4. Switch account in the remaining instance
  5. Close the second instance and open back the first one
  6. Make sure account is properly switched

@mkondratek
Copy link
Contributor

Works properly according to the test plan. ✅

Personally I do not like Scenario 6 and the constraint that I must have the same account in all the instances. I could have some conversation in one project and change the account in the other one. My discussion will disappear. Clearly, I can switch back to the original account and retrieve the chat from the history. However, it feels bad for me. I'd expect that account changes in one project does not affect my work in others.

Why is it needed to have the account switching logic so linked b/w the projects?

Copy link
Contributor

@mkondratek mkondratek left a comment

Choose a reason for hiding this comment

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

I do not like this behavior but I am almost sure that the scenario of having multiple accounts is very rare. Probably we are the ones playing with it the most.

Feel free to merge it if you are convinced.

@pkukielka
Copy link
Contributor Author

Why is it needed to have the account switching logic so linked b/w the projects?

Both options have pros and cons.
If we won't do that, then we should also require logging-in into each project separately, as mixing those models leads to confusion (e.g. what will happen when you sign-out)?
I think for most users 'login once - use everywhere' would be an expected behaviour.
Using different accounts on different IJ instances indeed seems a rare and a bit bizarre scenario.

@pkukielka pkukielka merged commit e8cbbc8 into main Sep 2, 2024
6 of 7 checks passed
@pkukielka pkukielka deleted the pkukielka/fix-account-login-logout-flow branch September 2, 2024 13:13
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