Skip to content

Conversation

@opieter-aws
Copy link
Contributor

@opieter-aws opieter-aws commented May 15, 2025

Problem

The auth state and region profile selection is not sycned between different IDE windows

Solution

Auth state

  • Add an ssoCacheWatcher to the LSP auth client, with hooks on onDidCreate and onDidDelete
  • In the AuthUtil, add the following handlers:
    • onDidCreate: trigger a restore flow to fetch the latest auth state
    • onDidDelete: trigger a logout

Region profile

  • Add a GlobalStatePoller util that polls the global state value every second, and call a handler if the value updates
  • In RegionProfileManager add the poller for the region profile global state variable and add a handler that switches the profile

Testing

Screen.Recording.2025-05-15.at.13.06.26.mov

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.

@opieter-aws opieter-aws changed the title Feature/amazonq lsp auth fix(amazonq): Sync IDE windows for Amazon Q auth state and region profile selection May 15, 2025
@opieter-aws opieter-aws reopened this May 15, 2025
@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@opieter-aws opieter-aws marked this pull request as ready for review May 15, 2025 17:14
@opieter-aws opieter-aws requested review from a team as code owners May 15, 2025 17:14
@hayemaxi
Copy link
Contributor

Very nice. Does it work if you say, select the profile in the other window that you weren't using to log in?

@hayemaxi
Copy link
Contributor

Do we need to poll every second? Maybe every 2 or 3 seconds?

@opieter-aws
Copy link
Contributor Author

opieter-aws commented May 15, 2025

Do we need to poll every second? Maybe every 2 or 3 seconds?

The behavior gets awkward when you are logged out and log back in; the 'other' window will keep the region selector displayed until the sync. I can increase to 2 seconds, but I don't think we want to keep that state much longer

Very nice. Does it work if you say, select the profile in the other window that you weren't using to log in?

Yes it behaves the same!

@nkomonen-amazon
Copy link
Contributor

nkomonen-amazon commented May 15, 2025

One thing to note with this polling length. We should think of realistic user behaviors and if we set polling to 2-3 seconds, would they even notice it? Assuming they sign in or out on one IDE instance, it is low likelihood they are switching to the other IDE very quickly.

It is also unlikely that the user will select the profile in one window and then immediately switch to the other.

@hayemaxi
Copy link
Contributor

One thing to note with this polling length. We should think of realistic user behaviors and if we set polling to 2-3 seconds, would they even notice it? Assuming they sign in or out on one IDE instance, it is low likelihood they are switching to the other IDE very quickly.

It is also unlikely that the user will select the profile in one window and then immediately switch to the other.

+1

No one is comparing how smooth the auth change is side-by-side.

@Will-ShaoHua
Copy link
Contributor

Will-ShaoHua commented May 15, 2025

thanks for this change, it's really nice. Maybe we should bring this to mainline as well

@opieter-aws
Copy link
Contributor Author

Maybe we should bring this to mainline as well

We won't be adding features for auth to mainline for now, but instead prioritize releasing this branch with auth on LSP identity server

@opieter-aws opieter-aws merged commit ed4b265 into aws:feature/amazonqLSP-auth May 15, 2025
14 of 22 checks passed
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.

4 participants