auto-trigger a/b #4961

andrewyuq · 2024-10-10T23:18:52Z

The configurable actions are added to the official release regardless(i.e. seeing them in the keymap), the difference is that for treatment group users they will be able to use the new configurable actions while the control group won't be able to use them.
Create mutliple xxxNew classes that will be used for new auto-trigger UX, old UX will continue to use the classes without the New suffix.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Description

Checklist

My code follows the code style of this project
I have added tests to cover my changes
A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
I have added metrics for my changes (if required)

License

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

Will-ShaoHua · 2024-10-14T15:17:46Z

...software/aws/toolkits/jetbrains/services/codewhisperer/editor/CodeWhispererEditorListener.kt

+                    if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+                        CodeWhispererInvocationStatusNew.getInstance().documentChanged()
+                    } else {
+                        CodeWhispererInvocationStatus.getInstance().documentChanged()


for all these singleton service, I think we can do

fun getInstance(): CodeWhispererService = if (newUX) CodeWhispererServiceNew.getInstance() else service()

CodeWhispererServiceNew.getInstance() is of type CodeWhispererServiceNew, so the getInstance() will have a type of Any then

Will-ShaoHua · 2024-10-14T15:54:45Z

...kits/jetbrains/services/codewhisperer/toolwindow/CodeWhispererCodeReferenceActionListener.kt

+        val manager = CodeWhispererCodeReferenceManager.getInstance(sessionContext.project)
+        manager.insertCodeReference(states, previews, sessionContext.selectedIndex)
+        manager.addListeners(sessionContext.editor)
+    }


https://github.com/aws/aws-toolkit-jetbrains/pull/4961/files#diff-1ab0dd85b16f44cfa6d69aa50fb4e94ae80cb9b76a3e9245b7fc93b1f3b29b82R717-R723

since we have the new implementation in the existing class, shouldnt we just use the existing CodeWhispererCodeReferenceActionListener above and override?

rli

i see lots of code that appears duplicate between old/new so it is difficult to figure out what is new and what is just the same logic but referencing new objects

rli · 2024-10-14T18:58:52Z

...e/aws/toolkits/jetbrains/services/codewhisperer/actions/CodeWhispererRecommendationAction.kt

+        if (!(
+                if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+                    CodeWhispererServiceNew.getInstance().canDoInvocation(editor, CodewhispererTriggerType.OnDemand)
+                } else {
+                    CodeWhispererService.getInstance().canDoInvocation(editor, CodewhispererTriggerType.OnDemand)
+                }
+                )
+        ) {
            return
        }

        val triggerType = TriggerTypeInfo(CodewhispererTriggerType.OnDemand, CodeWhispererAutomatedTriggerType.Unknown())
-        val job = CodeWhispererService.getInstance().showRecommendationsInPopup(editor, triggerType, latencyContext)
+        val job = if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+            CodeWhispererServiceNew.getInstance().showRecommendationsInPopup(editor, triggerType, latencyContext)
+        } else {
+            CodeWhispererService.getInstance().showRecommendationsInPopup(editor, triggerType, latencyContext)
+        }


can avoid working around typecast issues if you declare a common interface

rli · 2024-10-14T18:59:21Z

...ware/aws/toolkits/jetbrains/services/codewhisperer/credentials/CodeWhispererClientAdaptor.kt

when can we get rid of this file? it should be possible to have all sigv4/bearer coexist. ideally streamining/non-streaming can share clients too

rli · 2024-10-14T19:03:46Z

...ftware/aws/toolkits/jetbrains/services/codewhisperer/editor/CodeWhispererEditorManagerNew.kt

+        }
+    }
+
+    fun getMatchingSymbolsFromRecommendation(


if it's not too hard can you pull out the common code so there's less delta and risk

Will-ShaoHua · 2024-10-14T19:24:29Z

.../aws/toolkits/jetbrains/services/codewhisperer/telemetry/CodeWhispererTelemetryServiceNew.kt

+import java.util.Queue
+
+@Service
+class CodeWhispererTelemetryServiceNew {


doesn't telemetry will share most of the same logic and only have some new param, if only function parameters are different we can just have overloads

Will-ShaoHua · 2024-10-14T19:33:13Z

.../aws/toolkits/jetbrains/services/codewhisperer/telemetry/CodeWhispererCodeCoverageTracker.kt

+                CodeWhispererPopupManager.CODEWHISPERER_USER_ACTION_PERFORMED,
+                object : CodeWhispererUserActionListener {


looks like these 2 lines are shared, so we just need one listener with 2 afterAccept?

Will-ShaoHua · 2024-10-14T19:39:51Z

...oftware/aws/toolkits/jetbrains/services/codewhisperer/status/CodeWhispererStatusBarWidget.kt

+            if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+                CodeWhispererInvocationStatusNew.getInstance().hasExistingServiceInvocation()
+            } else {
+                CodeWhispererInvocationStatus.getInstance().hasExistingServiceInvocation()
+            }
+        ) {


same here if we can do

fun getInstqaonce() = if (newUX) CodeWhispererInvocationStatusNew.getInstance() else service()

code here won't need to be changed and can be as it is

CodeWhispererInvocationStatus.getInstance().hasExistingServiceInvocation()

Will-ShaoHua · 2024-10-14T19:48:24Z

...are/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererAutoTriggerService.kt

+            if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+                CodeWhispererTelemetryServiceNew.getInstance().previousUserTriggerDecision
+            } else {
+                CodeWhispererTelemetryService.getInstance().previousUserTriggerDecision


i feel telemetryService can be shared, and have likely maintain 2 previousUserTriggerDecision

class CwsprTelemetryService { private val previousUserTriggerDecisionOld: Queue<Decision> private val previousUserTrigerDecisionNew: Queue<Decision> val previousUserTriggerDecision: Decision get() { if (newUX) {} else {} } }

Will-ShaoHua · 2024-10-14T19:50:07Z

...are/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererAutoTriggerService.kt

+                if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+                    CodeWhispererServiceNew.getInstance().canDoInvocation(editor, CodewhispererTriggerType.AutoTrigger)
+                } else {
+                    CodeWhispererService.getInstance().canDoInvocation(editor, CodewhispererTriggerType.AutoTrigger)


same

fun getInstance() = if (newUX) newService.getInstance() else service()

then we won't need change here

CodeWhispererService.getInstance().canDoInvocation(editor, CodewhispererTriggerType.AutoTrigger)

Will-ShaoHua · 2024-10-14T19:51:03Z

...are/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererAutoTriggerHandler.kt

+        if (CodeWhispererFeatureConfigService.getInstance().getNewAutoTriggerUX()) {
+            CodeWhispererServiceNew.getInstance().showRecommendationsInPopup(editor, triggerTypeInfo, latencyContext)
+        } else {
+            CodeWhispererService.getInstance().showRecommendationsInPopup(editor, triggerTypeInfo, latencyContext)


same

fun getInstance() = if (newUX) newService.getInstance() else service()

then we won't need change here

CodeWhispererService.getInstance().showRecommendationsInPopup(...)

github-advanced-security

QDJVMC found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

This reverts commit b932c2c.

andrewyuq · 2024-10-17T19:18:35Z

Most of the comments are around refactoring old/new classses into having a common interface and implementations to implement the diffs, there's some work for the refactor, will be pushing the current version as is since it also does the job and revisit the refactor later

andrewyuq requested review from a team as code owners October 10, 2024 23:18

auto-trigger a/b

db17201

andrewyuq force-pushed the auto-trigger-review branch from bd19e86 to db17201 Compare October 10, 2024 23:21

andrewyuq added 3 commits October 10, 2024 16:30

fix compile error

fa88de2

detekt fix

cc11b69

Keep one topic for all the listeners and changes around coroutine

d994940

andrewyuq force-pushed the auto-trigger-review branch from 54710a6 to d994940 Compare October 11, 2024 18:34

andrewyuq and others added 5 commits October 11, 2024 11:49

test fix

33d4e08

detekt fix

2cb9b39

change force accept to shift+tab and remove opt+ enter

1303b46

keybinding change

e120665

Merge branch 'main' into auto-trigger-review

e3e519e

Will-ShaoHua reviewed Oct 14, 2024

View reviewed changes

rli approved these changes Oct 14, 2024

View reviewed changes

Will-ShaoHua approved these changes Oct 14, 2024

View reviewed changes

Will-ShaoHua reviewed Oct 14, 2024

View reviewed changes

andrewyuq and others added 2 commits October 17, 2024 11:01

Merge branch 'main' into auto-trigger-review

3e923de

change feature value to be string as service config has

4f23afb

github-advanced-security bot found potential problems Oct 17, 2024

View reviewed changes

andrewyuq added 3 commits October 17, 2024 11:47

test fix

b932c2c

Revert "test fix"

0bd02a8

This reverts commit b932c2c.

test fix

ee630fd

Merge branch 'main' into auto-trigger-review

450dbbf

andrewyuq enabled auto-merge (squash) October 17, 2024 19:29

andrewyuq merged commit 12e7803 into aws:main Oct 17, 2024
12 of 13 checks passed

		CodeWhispererPopupManager.CODEWHISPERER_USER_ACTION_PERFORMED,
		object : CodeWhispererUserActionListener {

auto-trigger a/b #4961

auto-trigger a/b #4961

Uh oh!

Conversation

andrewyuq commented Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

Description

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewyuq Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Will-ShaoHua Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Will-ShaoHua Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

andrewyuq commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrewyuq commented Oct 10, 2024 •

edited

Loading

andrewyuq Oct 14, 2024 •

edited

Loading

Will-ShaoHua Oct 14, 2024 •

edited

Loading

Will-ShaoHua Oct 14, 2024 •

edited

Loading

andrewyuq commented Oct 17, 2024 •

edited

Loading