Skip to content

Conversation

@Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Oct 8, 2024

Problem

temporarily undo most of: #5704

Solution

  • To re-bump we need to add changelog items, otherwise all work remains.
  • Considering adding a button to link to vscode download page, but thats an external link we don't control so holding off for now.
  • Without running it in the vsix, we have "null extension" in source field:
Screenshot 2024-10-08 at 4 03 32 PM
  • Notification in toolkit and Q:
Screenshot 2024-10-08 at 4 06 26 PM Screenshot 2024-10-08 at 4 07 23 PM

Steps to re-bump to 1.83.0

  • update vscode version in all package.json files.
  • add breaking fix changelog items in both extensions.

To-do: retest with vsix to make sure null source is gone.


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions
Copy link

github-actions bot commented Oct 8, 2024

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.

@Hweinstock Hweinstock marked this pull request as ready for review October 8, 2024 20:46
@Hweinstock Hweinstock requested a review from a team as a code owner October 8, 2024 20:46
}

// TODO: remove once version bump to 1.83.0 is complete.
export function setupVscodeVersionNotification() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need duplicate code?
AFAIK the null extension bug only impacts when running the extension in debug mode and not when its packaged into a vsix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, not aware of that. I can remove the duplicate code in that case.

}

if (notificationDisplayed) {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this ever run? Its always set to false on line 111.
Also it seems its only called once on activation anyways. This seems like more code than we need.

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 honestly wasn't sure, but I was using this as an example:

async function setupAuthNotification() {
let notificationDisplayed = false // Auth Notification should be displayed only once.
await tryShowNotification()
async function tryShowNotification() {
// Do not show the notification if the IDE starts and user is already authenticated.
if (AuthUtil.instance.isConnected()) {
notificationDisplayed = true
}
if (notificationDisplayed) {
return
}
const source = 'authNotification'
const buttonAction = 'Sign In'
notificationDisplayed = true
telemetry.toolkit_showNotification.emit({
component: 'editor',
id: source,
reason: 'notLoggedIn',
result: 'Succeeded',
})
const selection = await vscode.window.showWarningMessage('Start using Amazon Q', buttonAction)
if (selection === buttonAction) {
void amazonq.focusAmazonQPanel.execute(placeholder, source)
}
}
}
and left it in to be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

That example sorta makes sense because the state of notificationDisplayed can change after its initialized but before evaluation. But regardless it is a such a confusing and unnecessary way to evaluate the message. IMO we should refactor that.

@Hweinstock Hweinstock marked this pull request as draft October 8, 2024 21:20
@justinmk3
Copy link
Contributor

Oh darn, I should have mentioned I was working on this, see #5749 which also uses PromptSettings to store a "don't show again"


notificationDisplayed = true

telemetry.toolkit_showNotification.emit({
Copy link
Contributor

@justinmk3 justinmk3 Oct 8, 2024

Choose a reason for hiding this comment

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

seems like we should have a vscode.window.* wrapper in shared/utilities/messages.ts that emits this (maybe optional by a boolean flag). #5762

@justinmk3
Copy link
Contributor

#5749 has PromptSettings + tests so let's continue with that.

@justinmk3 justinmk3 closed this Oct 8, 2024
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.

3 participants