feat(amazonq): Prefetch next inline recommendation #5290
Conversation
evanliu048
changed the title
...y/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt
Outdated
Show resolved
Hide resolved
| } | ||
| val nextRecommendationContext = RecommendationContext(detailContexts, "", "", newVisualPosition) | ||
| val newPopup = withContext(EDT) { | ||
| JBPopupFactory.getInstance().createMessage("Dummy popup") |
There was a problem hiding this comment.
Is this ever shown to the user? Should this be a more user-friendly message?
There was a problem hiding this comment.
This dummy popup is not shown to the user. I created it as a placeholder since it's required for the nextInvocationContext variable. If there's a need to display a popup in the future, a new user-friendly popup will be created accordingly.
https://github.com/evanliu048/aws-toolkit-jetbrains/blob/prefechNextInlineRecommendation/plugins/amazonq/codewhisperer/jetbrains-community/src/software/aws/toolkits/jetbrains/services/codewhisperer/service/CodeWhispererService.kt#L745-L749
There was a problem hiding this comment.
can't we just handle the case where there is no prior popup there?
There was a problem hiding this comment.
The prefetch process requires creating a nextInvocationContext for the service, where the popup field cannot be null. To meet this requirement, I created a dummy popup. This popup is never actually displayed but serves as a placeholder to satisfy the non-null constraint.
If the creation of this dummy popup seems unusual, an alternative would be to make the popup field nullable. However, this would require adding null checks at every location where InvocationContext.popup is accessed, which would result in many changes any need to be tested thoroughly
To minimize the impact, maintain consistency and ensure this feature can be launched this week, I opted for this approach. If there are better suggestions for handling this scenario, I'm happy to discuss them!
There was a problem hiding this comment.
restating my request to delete this adapter if we have no need to make dataplane requests over sigv4
There was a problem hiding this comment.
Thank you for the clarification! I’m not entirely familiar with this part, so I’d appreciate some additional context. Could you elaborate on why removing this adapter is necessary?
There was a problem hiding this comment.
it is left over tech debt that's makes the client interactions more complicated. there is also a hidden footgun that can result in NullPointerException at runtime
There was a problem hiding this comment.
Thanks for pointing that out! I can discuss this further with our team to better understand the implications. Since this is outside the scope of the current feature, would it be possible to merge this PR first and address the issue in a separate follow-up? That way, I can ensure this feature is delivered while also properly handling the tech debt.
...ommunity/tst/software/aws/toolkits/jetbrains/services/codewhisperer/CodeWhispererTestBase.kt
Outdated
Show resolved
Hide resolved
...ity/tst/software/aws/toolkits/jetbrains/services/codewhisperer/CodeWhispererTelemetryTest.kt
Outdated
Show resolved
Hide resolved
| nextInvocationContext?.popup?.let { Disposer.dispose(it) } | ||
| nextInvocationContext = null | ||
|
|
||
| runInEdt { |
There was a problem hiding this comment.
if we're already using coroutines, we do not need to use runInEdt
There was a problem hiding this comment.
Thank you for pointing that out! I use runInEdt in this context because several operations, such as initializing the popup (initPopup) and updating its listeners, involve UI changes that must be executed on the Event Dispatch Thread (EDT) to ensure thread safety and proper behavior. In similar cases that I’ve come across, runInEdt is consistently used, so I opted to keep it here as a safeguard.
There was a problem hiding this comment.
withContext(EDT) has the same behavior has runInEdt, so it does not make sense to mix executors that move actions onto other threads with coroutines
There was a problem hiding this comment.
Thanks for the clarification! Just to confirm—are you suggesting that I replace runInEdt with withContext(EDT), or do you mean I should simply remove runInEdt without any replacement? Want to make sure I fully understand your expectation before making the update. Thanks!
| val firstValidRecommendation = currStates.recommendationContext.details | ||
| .firstOrNull { | ||
| !it.isDiscarded && it.recommendation.content().isNotEmpty() | ||
| } ?: return SupervisorJob().apply { complete() } |
There was a problem hiding this comment.
does this actually need to return a job?
There was a problem hiding this comment.
The function's return type is explicitly defined as Job, and there is an existing test that relies on this return value. So I return a SupervisorJob to maintain consistency with current codebase.
https://github.com/evanliu048/aws-toolkit-jetbrains/blob/prefechNextInlineRecommendation/plugins/amazonq/codewhisperer/jetbrains-community/tst/software/aws/toolkits/jetbrains/services/codewhisperer/CodeWhispererServiceTest.kt#L219
| } | ||
| val nextRecommendationContext = RecommendationContext(detailContexts, "", "", newVisualPosition) | ||
| val newPopup = withContext(EDT) { | ||
| JBPopupFactory.getInstance().createMessage("Dummy popup") |
There was a problem hiding this comment.
can't we just handle the case where there is no prior popup there?
…/aws/toolkits/jetbrains/services/codewhisperer/CodeWhispererTelemetryTest.kt Co-authored-by: Richard Li <[email protected]>
…/aws/toolkits/jetbrains/services/codewhisperer/CodeWhispererTestBase.kt Co-authored-by: Richard Li <[email protected]>
Types of changes
Description
This PR introduces the ability for users to see the next recommendation immediately after accepting the current one, provided there is a subsequent recommendation available. The key enhancements include:
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.