Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions packages/amazonq/src/developerProfile/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import * as nls from 'vscode-nls'
const localize = nls.loadMessageBundle()
import vscode from 'vscode'
import { AmazonQPromptSettings, placeholder } from 'aws-core-vscode/shared'
import { telemetry } from 'aws-core-vscode/telemetry'
import { selectRegionProfileCommand, toastMessage } from 'aws-core-vscode/codewhisperer'
import { once } from 'aws-core-vscode/utils'

/**
* Creates a toast message telling the user they need to select a Developer Profile
*/
export const notifySelectDeveloperProfile = once(_notifySelectDeveloperProfile)
async function _notifySelectDeveloperProfile() {
const suppressId = 'amazonQSelectDeveloperProfile'
const settings = AmazonQPromptSettings.instance
const shouldShow = settings.isPromptEnabled(suppressId)
if (!shouldShow) {
return
}

const message = localize(
'aws.amazonq.profile.mustSelectMessage',
'You must select a Q Developer Profile for Amazon Q features to work.'
)
const selectProfile = 'Select Profile'
const dontShowAgain = 'Dont Show Again'

await telemetry.toolkit_showNotification.run(async () => {
telemetry.record({ id: 'mustSelectDeveloperProfileMessage' })
void vscode.window.showWarningMessage(message, selectProfile, dontShowAgain).then(async (resp) => {
await telemetry.toolkit_invokeAction.run(async () => {
if (resp === selectProfile) {
// Show Profile
telemetry.record({ action: 'select' })
void selectRegionProfileCommand.execute(placeholder, toastMessage)
} else if (resp === dontShowAgain) {
telemetry.record({ action: 'dontShowAgain' })
await settings.disablePrompt(suppressId)
} else {
telemetry.record({ action: 'ignore' })
}
})
})
})
}
2 changes: 0 additions & 2 deletions packages/amazonq/src/lsp/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ import vscode from 'vscode'
import { startLanguageServer } from './client'
import { AmazonQLspInstaller } from './lspInstaller'
import { lspSetupStage, ToolkitError, messages } from 'aws-core-vscode/shared'
import { AuthUtil } from 'aws-core-vscode/codewhisperer'

export async function activate(ctx: vscode.ExtensionContext) {
try {
await lspSetupStage('all', async () => {
const installResult = await new AmazonQLspInstaller().resolve()
return await lspSetupStage('launch', () => startLanguageServer(ctx, installResult.resourcePaths))
})
await AuthUtil.instance.restore()
} catch (err) {
const e = err as ToolkitError
void messages.showViewLogsMessage(`Failed to launch Amazon Q language server: ${e.message}`)
Expand Down
17 changes: 15 additions & 2 deletions packages/amazonq/src/lsp/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,23 @@ export async function startLanguageServer(
toDispose.push(disposable)

await client.onReady()

// IMPORTANT: This sets up Auth and must be called before anything attempts to use it
AuthUtil.create(new auth2.LanguageClientAuth(client, clientId, encryptionKey))
// Ideally this would be part of AuthUtil.create() as it restores the existing Auth connection, but we have some
// future work which will need to delay this call
await AuthUtil.instance.restore()

await postStartLanguageServer(client, resourcePaths, toDispose)

return client
}

async function postStartLanguageServer(
client: LanguageClient,
resourcePaths: AmazonQResourcePaths,
toDispose: vscode.Disposable[]
) {
// Request handler for when the server wants to know about the clients auth connnection. Must be registered before the initial auth init call
client.onRequest<ConnectionMetadata, Error>(auth2.notificationTypes.getConnectionMetadata.method, () => {
return {
Expand Down Expand Up @@ -347,8 +362,6 @@ export async function startLanguageServer(
// Set this inside onReady so that it only triggers on subsequent language server starts (not the first)
onServerRestartHandler(client)
)

return client
}

/**
Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/auth/auth2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -325,10 +325,14 @@ export class SsoLogin implements BaseLogin {
}

private updateConnectionState(state: AuthState) {
if (this.connectionState !== state) {
this.eventEmitter.fire({ id: this.profileName, state })
const oldState = this.connectionState
const newState = state

this.connectionState = newState

if (oldState !== newState) {
this.eventEmitter.fire({ id: this.profileName, state: this.connectionState })
Comment on lines +331 to +334
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there no race conditions in swapping the event emitter with the status update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is unlikely (maybe impossible in this case), but when we fire the event emitter and have not updated this.connectionState to the correct value yet, something may attempt to read it after getting the event and it will be a stale value.

I made this change initially though with the assumption that it may have had no actual impact.

}
this.connectionState = state
}

private ssoTokenChangedHandler(params: SsoTokenChangedParams) {
Expand Down
5 changes: 0 additions & 5 deletions packages/core/src/codewhisperer/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ import { setContext } from '../shared/vscode/setContext'
import { syncSecurityIssueWebview } from './views/securityIssue/securityIssueWebview'
import { detectCommentAboveLine } from '../shared/utilities/commentUtils'
import { activateEditTracking } from './nextEditPrediction/activation'
import { notifySelectDeveloperProfile } from './region/utils'

let localize: nls.LocalizeFunc

Expand Down Expand Up @@ -351,10 +350,6 @@ export async function activate(context: ExtContext): Promise<void> {
await AuthUtil.instance.notifySessionConfiguration()
}
}

if (AuthUtil.instance.regionProfileManager.requireProfileSelection()) {
await notifySelectDeveloperProfile()
}
},
{ emit: false, functionId: { name: 'activateCwCore' } }
)
Expand Down
30 changes: 27 additions & 3 deletions packages/core/src/codewhisperer/util/authUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import { builderIdStartUrl, internalStartUrl } from '../../auth/sso/constants'
import { VSCODE_EXTENSION_ID } from '../../shared/extensions'
import { RegionProfileManager } from '../region/regionProfileManager'
import { AuthFormId } from '../../login/webview/vue/types'
import { notifySelectDeveloperProfile } from '../region/utils'
import { once } from '../../shared/utilities/functionUtils'

const localize = nls.loadMessageBundle()

Expand Down Expand Up @@ -91,8 +93,20 @@ export class AuthUtil implements IAuthProvider {
return this.session.loginType === LoginTypes.SSO
}

/**
* HACK: Ideally we'd put {@link notifySelectDeveloperProfile} in to {@link restore}.
* But because {@link refreshState} is only called if !isConnected, we cannot do it since
* {@link notifySelectDeveloperProfile} needs {@link refreshState} to run so it can set
* the Bearer Token in the LSP first.
*/
startedConnected = false
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the name startedConnected is misleading, since the AuthUtil is always instantiated with the state notConnected. I was thinking, and maybe it's clearer to not reference an AuthState here. Instead, consider renaming to startedSignedOut or startedSignedIn, which is more descriptive and avoids confusion with auth state. Then you can change line 110 to assert the same variable, which also makes it clearer what we actually do there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good improvement, will update


async restore() {
await this.session.restore()
this.startedConnected = this.isConnected()

// HACK: The LSP should allow us to call refreshState multiple times, but for some reason it breaks.
// isConnected somehow allows it to work. Assumption is that the LSP does not handle redundant calls nicely.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rephrase to: 'this check ensures we only call refreshState when restore does not succeed to connect, to avoid duplicate calls', since we do know why it works

Copy link
Contributor Author

@nkomonen-amazon nkomonen-amazon May 13, 2025

Choose a reason for hiding this comment

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

IMO we shouldn't care about duplicate calls of refreshState since it should functionally work if called multiple times. If refreshState is fragile it should be documented somewhere

Also in restore() I would assume that after calling it, the whole auth state is "ready to go", but it isn't since we need to wait for the event emitter to implicitly trigger refreshState.

I'm going to reword the comment though to word this better

if (!this.isConnected()) {
await this.refreshState()
}
Expand Down Expand Up @@ -257,20 +271,24 @@ export class AuthUtil implements IAuthProvider {

private async refreshState(state = this.getAuthState()) {
if (state === 'expired' || state === 'notConnected') {
this.lspAuth.deleteBearerToken()
if (this.isIdcConnection()) {
await this.regionProfileManager.invalidateProfile(this.regionProfileManager.activeRegionProfile?.arn)
await this.regionProfileManager.clearCache()
}
this.lspAuth.deleteBearerToken()
}
if (state === 'connected') {
const bearerTokenParams = (await this.session.getToken()).updateCredentialsParams
await this.lspAuth.updateBearerToken(bearerTokenParams)

if (this.isIdcConnection()) {
await this.regionProfileManager.restoreProfileSelection()
}
const bearerTokenParams = (await this.session.getToken()).updateCredentialsParams
await this.lspAuth.updateBearerToken(bearerTokenParams)
}

// regardless of state, send message at startup if user needs to select a Developer Profile
void this.tryNotifySelectDeveloperProfile()

Comment on lines +299 to +301
Copy link
Contributor

Choose a reason for hiding this comment

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

This refreshState function triggers not only upon startup, but on every auth state change. Do we need to check here whether it's the first use or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function, see below, only triggers once on startup, and any subsequent times it will not because of the use of once

vsCodeState.isFreeTierLimitReached = false
await this.setVscodeContextProps(state)
await Promise.all([
Expand All @@ -283,6 +301,12 @@ export class AuthUtil implements IAuthProvider {
}
}

private tryNotifySelectDeveloperProfile = once(async () => {
if (this.regionProfileManager.requireProfileSelection() && this.startedConnected) {
await notifySelectDeveloperProfile()
}
})

async getTelemetryMetadata(): Promise<TelemetryMetadata> {
if (!this.isConnected()) {
return {
Expand Down