-
Notifications
You must be signed in to change notification settings - Fork 274
fix(amazonq): reauth workflow should not prompt profile selection page #5549
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
Conversation
# Conflicts: # plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/profile/QRegionProfileManager.kt
rli
left a comment
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.
this is probably worth a test case
| if (connection is AwsBearerTokenConnection && !connection.isSono()) { | ||
| return connection | ||
| val manager = ToolkitConnectionManager.getInstance(project) | ||
| val connection = manager.activeConnectionForFeature(QConnection.getInstance()) as? AwsBearerTokenConnection |
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.
i think at this point we need to completely remove CodeWhispererConnection if we are assuming everything is now QConnection
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.
I may be missing part of the context, but I believe I'm using QConnection here—let me know if I'm misunderstanding anything
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.
because there was a old CodeWhispererConnection.kt before QConnection came in, so ideally we will check both connection for backward compatiblity. You can check isQConnected. I am in favor of removing codewhispererConnection as it would be a burden to check 2 connection type everywhere and considering we've been using Q_Scope for roughly 1 year, it shouldn't harm to remove CwsprConnection. I can follow up on the cwspr connectin deprecation if @rli you don't have concern about the deprecation
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.
Thanks for the clarification Will. We would need a separate pr to handle the codewhisperer connection deprecation right
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.
yes
Problem
On IDE startup,
QRegionProfileManagerloads the active profile fromaws.xml.However,
validateProfile()is called before SSO reauth completes, causinglistRegionProfiles()to return empty. This results in the active profile being reset tonull.Fix
Update
getIdcConnectionOrNull()to return the connection only if it's a valid Q connection and the token is authorized.Also, after successful reauth, immediately show the Q panel to avoid triggering the profile selection flow.
Summary
Changes
getIdcConnectionOrNull()to check forBearerTokenAuthState.AUTHORIZEDThis should improve the IDE startup flow for users who rely on SSO, reducing friction and avoiding false-positive "profile selection required" states.
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.