Skip to content
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

Merged
merged 93 commits into from
Oct 24, 2024

Conversation

tverney
Copy link
Contributor

@tverney tverney commented Sep 26, 2024

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:

  • RTS is already supporting the change on prod
  • codeGenerationId and uploadId are now being generated in FE
  • AppSec and pentest passed successfully
  • Telemetry is included in this PR and strings are reviewed/approved by Technical writer
  • This stop action is only introduced in /dev feature. This did not enable in any other product for this extension.

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

Copy link

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

@tverney tverney force-pushed the feature/stop-progress-status branch from 0588d8e to e604672 Compare September 27, 2024 17:42
Copy link

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.

@tverney tverney marked this pull request as ready for review October 16, 2024 14:13
hayemaxi and others added 12 commits October 21, 2024 14:32
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.
@tverney tverney requested a review from a team as a code owner October 21, 2024 19:15
@tverney tverney requested a review from neilk-aws October 22, 2024 19:38
@tverney
Copy link
Contributor Author

tverney commented Oct 22, 2024

We have already the security sign-off. Ready to be reviewed cc: @neilk-aws

packages/core/src/shared/telemetry/vscodeTelemetry.json Outdated Show resolved Hide resolved
@@ -46,6 +46,7 @@ export class FollowUpInteractionHandler {
if (followUp.prompt !== undefined) {
this.mynahUI.updateStore(tabID, {
loadingChat: true,
cancelButtonWhenLoading: false,
Copy link
Contributor

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@tverney tverney Oct 24, 2024

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.

tverney and others added 3 commits October 23, 2024 11:13
@tverney tverney requested a review from jpinkney-aws October 23, 2024 20:34
@tverney tverney requested a review from hayemaxi October 24, 2024 15:01
@hayemaxi hayemaxi merged commit 3b20744 into aws:master Oct 24, 2024
21 of 24 checks passed
@tverney tverney deleted the feature/stop-progress-status branch October 24, 2024 15:17
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.