fix(amazonq): Developer Profiles not being sent to Flare Auth as expected #7293
Conversation
This may not actually do anything, but we risked something reading the stale state after emitting the event because we set it after the fire(). Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
The Auth LSP complains if we set the developer profile before setting the bearer token, because it assumes we should not be setting a profile if a token does not exist. So to fix this, we first ensure the server is updated with the token. Signed-off-by: nkomonen-amazon <[email protected]>
Problem: THe show developer profile message was appearing at the wrong time. It was due to the order that the code executed. This was because when we called to check if the profiles were restored from cache, the logic that restored them had not actually run yet. Solution: Move the logic that decides to show the Needs Profile message, to after the code that restores the Profile. It is also only called once since we only want to show the message on startup. Signed-off-by: nkomonen-amazon <[email protected]>
| this.connectionState = newState | ||
|
|
||
| if (oldState !== newState) { | ||
| this.eventEmitter.fire({ id: this.profileName, state: this.connectionState }) |
There was a problem hiding this comment.
Are there no race conditions in swapping the event emitter with the status update?
There was a problem hiding this comment.
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.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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| * {@link notifySelectDeveloperProfile} needs {@link refreshState} to run so it can set | ||
| * the Bearer Token in the LSP first. | ||
| */ | ||
| startedConnected = false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good improvement, will update
- Fix docstring explaining refreshState fragility - Fix variable name Signed-off-by: nkomonen-amazon <[email protected]>
|
nkomonen-amazon
changed the title
Signed-off-by: nkomonen-amazon <[email protected]>
Signed-off-by: nkomonen-amazon <[email protected]>
| // regardless of state, send message at startup if user needs to select a Developer Profile | ||
| void this.tryNotifySelectDeveloperProfile() | ||
|
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The function, see below, only triggers once on startup, and any subsequent times it will not because of the use of once
opieter-aws
merged commit f79defd
into
aws:feature/amazonqLSP-auth
2 participants
Problem
Due to the order that we restored auth, versus sent the Developer Profiles to the Auth LSP, there was a problem. We sent the Developer Profiles before the Bearer Token was setup in the Amazon Q LSP
Solution
Ensure that we complete restoring of Auth (pushing the Bearer Token to the Amazon Q LSP). Then restore the Developer Profile, ensuring we show the correct messages if needed.
To Review
See each individual commit for a breakdown as their are additional refactors.
feature/xbranches will not be squash-merged at release time.