-
Notifications
You must be signed in to change notification settings - Fork 273
feat(amazonq): add support for CodeReview and DisplayFindings tools, alter behavior of explain and fix buttons #5970
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
Conversation
…alter behavior of explain and fix buttons
...s-community/src/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnector.kt
Fixed
Show fixed
Hide fixed
...s-community/src/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnector.kt
Fixed
Show fixed
Hide fixed
...s-community/src/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnector.kt
Fixed
Show fixed
Hide fixed
...s-community/src/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnector.kt
Fixed
Show fixed
Hide fixed
...re/aws/toolkits/jetbrains/services/cwc/commands/codescan/actions/HandleIssueCommandAction.kt
Fixed
Show fixed
Hide fixed
.../toolkits/jetbrains/services/codewhisperer/codescan/utils/CodeWhispererCodeScanIssueUtils.kt
Fixed
Show fixed
Hide fixed
...c/software/aws/toolkits/jetbrains/services/amazonq/lsp/flareChat/ChatCommunicationManager.kt
Fixed
Show fixed
Hide fixed
...unity/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/model/aws/chat/ChatMessage.kt
Fixed
Show fixed
Hide fixed
...unity/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/model/aws/chat/ChatMessage.kt
Fixed
Show fixed
Hide fixed
…alter behavior of explain and fix buttons
… Path.toUri() for WSL (aws#5960) Fix repeated UriError when Amazon Q starts with a WSL-backed workspace. We now validate the constructed file: URI and fallback to JDK’s Path.toUri() if the URI violates Rule (no authority while the path begins with //, maybe RFC 3986). This keeps legacy behavior where it’s safe, and prevents Q from rejecting the workspace folder URI
…5894) We need to support client-side opt-out for internal users who have workspace context server running by default.
It is not actually needed; we can read the values from AmazonQLspService
…alter behavior of explain and fix buttons
# Conflicts: # plugins/amazonq/shared/jetbrains-community/src/software/aws/toolkits/jetbrains/services/amazonq/lsp/flareChat/AwsServerCapabilitiesProvider.kt
...c/software/aws/toolkits/jetbrains/services/amazonq/lsp/flareChat/ChatCommunicationManager.kt
Fixed
Show fixed
Hide fixed
...c/software/aws/toolkits/jetbrains/services/amazonq/lsp/flareChat/ChatCommunicationManager.kt
Fixed
Show fixed
Hide fixed
… tools, alter behavior of explain and fix buttons" This reverts commit f21c028.
/retryBuild |
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.
General comment - I dont see anything specific to telemetry events for Explain and Apply Fix. I believe all other events are coming from LangaugeServer.
throw error | ||
} | ||
chatCommunicationManager.removePartialChatMessage(partialResultToken) | ||
val params = value?.let { encryptionManager?.decrypt(it) }.orEmpty() |
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.
Is this is in response to specific message from the UI? can we please create a separate function for this? Should this be included in every chat result?
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.
I will move it to a separate function
fun createLineRangeText(issueContext: MutableMap<String, String>): String { | ||
val startLineString = issueContext["startLine"] | ||
val endLineString = issueContext["endLine"] | ||
val startLineInteger = Integer.parseInt(startLineString) |
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.
can we use toInt()
val endLineString = issueContext["endLine"] | ||
val startLineInteger = Integer.parseInt(startLineString) | ||
val endLineInteger = Integer.parseInt(endLineString) | ||
return if (startLineInteger == endLineInteger) { |
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.
Any specific reason we return a string and not a list here?
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 return value is only used in formatting a string, so it makes sense to return string here
) { | ||
val uiReady = CompletableDeferred<Boolean>() | ||
private val chatCommunicationManager = ChatCommunicationManager.getInstance(project) | ||
private val codeScanManager = CodeWhispererCodeScanManager.getInstance(project) |
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.
recommendation is don't store this as a field
/retryBuild |
} | ||
|
||
private fun parseFindingsMessages(messagesMap: Map<*, *>) { | ||
val additionalMessages = messagesMap["additionalMessages"] as? MutableList<Map<String, Any>> |
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.
can we please add this in a try catch block?
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.
will do
} | ||
} | ||
|
||
private fun parseFindingsMessages(messagesMap: Map<*, *>) { |
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.
Can we add tests for this please?
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.
will try to do this- need to look into how unit tests are written
} ?: partialChatResult | ||
|
||
if (partialResultMap is Map<*, *>) { | ||
val additionalMessages = partialResultMap["additionalMessages"] as? MutableList<Map<String, Any>> |
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.
will additionalMessages always point to code review findings?
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 messages in additional messages can be any message, but we filter by messageId to remove code review findings from partial results map. This prevents the findings from appearing in the chat
Is there a changelog required here? |
This feature needs a changeLog, @BlakeLazarine Can you add this? |
} | ||
} | ||
|
||
fun parseFindingsMessages(messagesMap: Map<*, *>) { |
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.
model types properly instead of turning everything into maps and casting everywhere
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.
I am going to push back on this because there are many possible datatypes that the fields in this can take, and it would be difficult to guarantee that they are all caught by an absolute type. Any missed datatype would cause a failure, when we really only care about the additionalMessages field
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.
how we do this is pass-through the entire payload to the UI<->LSP channel and only model the fields we want to process. then we can let the deserializer drop anything we don't care about, but not affect the underlying communication
val jsonFindings = gson.fromJson(findingsMessage["body"] as String, List::class.java) | ||
val mappedFindings = mutableListOf<CodeWhispererCodeScanIssue>() | ||
for (aggregatedIssueUnformatted in jsonFindings) { | ||
val aggregatedIssue = gson.fromJson(gson.toJson(aggregatedIssueUnformatted), AggregatedCodeScanIssue::class.java) |
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.
?
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.
We are receiving a List of AggregatedCodeScanIssues as a string from FLARE message, this is formatting them and putting them into a List object.
val contextDataKey = DataKey.create<MutableMap<String, String>>("amazonq.codescan.handleIssueCommandContext") | ||
val actionDataKey = DataKey.create<String>("amazonq.codescan.handleIssueCommandAction") |
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.
data keys should be created once, not every time one is needed
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.
moved to common
...rc/software/aws/toolkits/jetbrains/services/cwc/controller/chat/telemetry/TelemetryHelper.kt
Show resolved
Hide resolved
...tware/aws/toolkits/jetbrains/services/codewhisperer/codescan/CodeWhispererCodeScanManager.kt
Outdated
Show resolved
Hide resolved
...mmunity/tst/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnectorTest.kt
Outdated
Show resolved
Hide resolved
...s-community/src/software/aws/toolkits/jetbrains/services/amazonq/webview/BrowserConnector.kt
Fixed
Show fixed
Hide fixed
import software.aws.toolkits.jetbrains.services.cwc.controller.chat.telemetry.TelemetryHelper | ||
import software.aws.toolkits.jetbrains.services.cwc.controller.chat.telemetry.getStartUrl | ||
|
||
class HandleIssueCommandAction : AnAction(), DumbAware { |
Check warning
Code scanning / QDJVMC
Component/Action not registered Warning
if (findingsMessages != null) { | ||
for (findingsMessage in findingsMessages) { | ||
additionalMessages.remove(findingsMessage) | ||
val gson = Gson() |
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.
nit: would recommend using the same instance of Gson throughout the class
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.
will add in follow up PR
|
||
fun parseFindingsMessages(messagesMap: Map<String, *>) { | ||
try { | ||
val additionalMessages = messagesMap["additionalMessages"] as? MutableList<Map<String, Any>> |
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.
Why is this any?
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 typing is very complex and only the shape of additionalMessages matters. Even then I am not confident in my ability to know the shape of every possible entry here. Only the shape of the findings message matters. Introducing forced shapes would be too risky in my opinion
val contextDataKey = DataKey.create<MutableMap<String, String>>("amazonq.codescan.handleIssueCommandContext") | ||
val actionDataKey = DataKey.create<String>("amazonq.codescan.handleIssueCommandAction") |
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.
val contextDataKey = DataKey.create<MutableMap<String, String>>("amazonq.codescan.handleIssueCommandContext") | |
val actionDataKey = DataKey.create<String>("amazonq.codescan.handleIssueCommandAction") |
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.
will add in follow up PR
Alters JB plugin to support CodeReview and DisplayFindings tools which are already present in flare.
This enables the Q agent to use the Q Code Review functionality and also migrates /review to call the agent.
The IssueDetails panel has been modified to remove the generated fix, and instead just has options for Explain and Fix, which send information about the issue to the Q Chat.
Types of changes
Description
Checklist
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.