-
Notifications
You must be signed in to change notification settings - Fork 487
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
feat(dev): Stop code generation action #5675
Conversation
This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions. |
…rence for stop code generation
…tance and abort the individual call
0588d8e
to
e604672
Compare
…cancellation should be in gen
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. |
Add stop code gen telemetry
Problem: c8 was not given a reporter, therefore it defaulted to `text` which is the terminal. Solution: Set it to `lcov`, which is what we have been using previously. Note: More work may be needed on the .c8rc.json files. We might not need the one for core/ anymore. They may not be configured in the optimal way. Is it behaving how we want it to? --- <!--- REMINDER: Ensure that your PR meets the guidelines in CONTRIBUTING.md --> License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
…rence for stop code generation
…tance and abort the individual call
…cancellation should be in gen
We have already the security sign-off. Ready to be reviewed cc: @neilk-aws |
@@ -46,6 +46,7 @@ export class FollowUpInteractionHandler { | |||
if (followUp.prompt !== undefined) { | |||
this.mynahUI.updateStore(tabID, { | |||
loadingChat: true, | |||
cancelButtonWhenLoading: false, |
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.
Out of curiosity does this mean that every single update store will need this? Why isn't the default false and then its true when we want the cancel button (which I assume is the stop button) to show?
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.
The change itself kind of scares me because we need to make sure its propagated everything and that everyone who makes changes to the updateStore knows why it exists
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.
@jpinkney-aws Refer to the section on MynahUI https://github.com/aws/mynah-ui/blob/0f47cdf4dcaf2574ec118cdbe7c5cdb9339d4f6d/docs/DATAMODEL.md?plain=1#L38
"In addition to the spinner, if onStopChatResponse
is attached globally through MynahUI main class constructor properties (see Constructor properties for details) and cancelButtonWhenLoading
is not set to false specifically for that tab it will show the stop generating button too."
You can sync with @dogusata about this specifically.
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.
If none of the functionalities are supporting onStopChatResponse
why we have it attached to the mynahui instance? If some functions are using it, then this change makes sense. However the defaults can be changed for newly generated tabs. But i don't see any specific difference between giving true to the tabs which will using it or false to tabs which will not.
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.
Additionally, giving the value during the tab generation is sufficient. Also, until send a new value, the previous value will remain.
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.
@jpinkney-aws do we have a consensus about this specific ? Or do we need sync about it? I'm ok for now with @dogusata, no one its using it, I did safe check in all the places in this PR that we use the store.
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.
So just to confirm - state of cancelButtonWhenLoading = false
must be passed if onStopChatResponse
was specified earlier, otherwise it defaults to true. If onStopChatResponse
wasn't passed, then it doesn't matter.
If this is correct then the current implementation is fine IMO.
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.
Exactly, that's the behaviour. If onStopChatResponse isn't passed, it defaults to false. Otherwise, need explicitly say false to not show it.
packages/core/src/amazonqFeatureDev/controllers/chat/controller.ts
Outdated
Show resolved
Hide resolved
packages/core/src/amazonqFeatureDev/controllers/chat/controller.ts
Outdated
Show resolved
Hide resolved
telemetry(amazonq): use ui_click telemetry for stop code generate button instead
Problem
Currently /dev don't support stop code generation. This PR introduces this functionality (watch the video below).
Solution
Mynah UI provides onStopChatResponse API which we can hook in the cancellation token provided on VS Code, sharing across an active session, aborting current progress.
Screen.Recording.2024-10-16.at.10.11.20.AM.mov
Notes:
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.