feat(amazonq): add button to view diff in IDE #5338

ivikash · 2024-07-18T05:45:34Z

Add Accept Diff & View Diff button to Amazon Q for auto generated code.

Screen.Recording.2024-10-02.at.9.10.45.AM.mov

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

justinmk3 · 2024-07-22T12:16:44Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+            const fileName = path.basename(right.path)
+
+            const diffScheme = 'amazon-q-accept-diff'
+            const left = Uri.parse(`${diffScheme}:${fileName}-` + randomUUID())


getNonexistentFilename may be useful

aws-toolkit-vscode/packages/core/src/shared/filesystemUtilities.ts

Line 189 in 1c1b0bf

export async function getNonexistentFilename(

justinmk3

which Q team needs to review this?

github-actions · 2024-09-19T15:25:17Z

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.

packages/core/src/amazonq/commons/diff.ts

justinmk3 · 2024-10-02T17:24:39Z

packages/core/src/amazonq/commons/diff.ts

+ * @param {vscode.Selection} selection - The selection range in the document.
+ * @returns {vscode.Range} - The VSCode range object representing the start and end of the selection.
+ */
+export function getSelectionFromRange(doc: vscode.TextDocument, selection: vscode.Selection) {


packages/core/src/shared/utilities/textDocumentUtilities.ts

packages/core/src/amazonq/commons/diff.ts

justinmk3 · 2024-10-02T17:30:39Z

packages/core/src/amazonq/commons/diff.ts

for better or worse, the "commons" concept is named shared/ in this codebase. can we avoid introducing a different name?

q-specific common modules should live in packages/core/src/shared/amazonq/ if they are truly Q-only and can't live in the more specific packages/amazonq/src/shared/

justinmk3 · 2024-10-02T17:33:18Z

packages/core/src/codewhispererChat/controllers/chat/telemetryHelper.ts

+                    cwsprChatHasProjectContext: this.responseWithProjectContext.get(message.messageId),
+                }
+                break
+            case 'view_diff':


nit: seems strange that this uses snake_case, acceptDiff is camelCase and the others are hyphen-words

Indeed thats very true. Even during development its weird.

justinmk3 · 2024-10-02T17:47:34Z

packages/core/src/shared/constants.ts

 export const amazonQVscodeMarketplace =
    'https://marketplace.visualstudio.com/items?itemName=AmazonWebServices.amazon-q-vscode'
+
+export const amazonQDiffScheme = 'amazon-q-view-diff'


we probably need a single schemes.fs file somewhere, but until we decide that, this can at least live next to the existing ones:

aws-toolkit-vscode/packages/core/src/shared/constants.ts

Lines 111 to 114 in 80ac383

* URI scheme for CloudWatch Logs Virtual Documents

*/

export const CLOUDWATCH_LOGS_SCHEME = 'aws-cwl' // eslint-disable-line @typescript-eslint/naming-convention

export const AWS_SCHEME = 'aws' // eslint-disable-line @typescript-eslint/naming-convention

justinmk3 · 2024-10-04T20:30:53Z

packages/core/src/shared/constants.ts

 */
 export const CLOUDWATCH_LOGS_SCHEME = 'aws-cwl' // eslint-disable-line @typescript-eslint/naming-convention
 export const AWS_SCHEME = 'aws' // eslint-disable-line @typescript-eslint/naming-convention
+export const AMAZON_Q_DIFF_SCHEME = 'amazon-q-view-diff' // eslint-disable-line @typescript-eslint/naming-convention


eslint-disable-line was a compromise to avoid code churn, not intended to be continued for new symobls

is "view" an imporant thing to mention in the scheme name?

Suggested change

export const AMAZON_Q_DIFF_SCHEME = 'amazon-q-view-diff' // eslint-disable-line @typescript-eslint/naming-convention

export const amazonQDiffScheme = 'amazon-q-diff'

justinmk3 · 2024-10-04T20:32:12Z

packages/core/src/shared/utilities/textDocumentUtilities.ts

+    const languageId = (await vscode.workspace.openTextDocument(originalFileUri)).languageId
+    const tempFile = _path.parse(originalFileUri.path)
+    const tempFilePath = _path.join(tempDirPath, `${tempFile.name}_proposed-${id}${tempFile.ext}`)
+    const tempFileUri = vscode.Uri.parse(`${AMAZON_Q_DIFF_SCHEME}:${tempFilePath}`)


scheme should probably be a parameter, but maybe that can be revisited later.

justinmk3 · 2024-10-04T20:34:35Z

packages/core/src/amazonq/webview/generators/webViewContent.ts

+        try {
+            await FeatureConfigProvider.instance.fetchFeatureConfigs()
+            featureConfigs = FeatureConfigProvider.getFeatureConfigs()
+        } catch (error) {}


should it not at least log a warning?

justinmk3 · 2024-10-04T20:37:05Z

packages/core/src/amazonq/util/functionUtils.ts

+ * const map = deserializeArrayToMap(array);
+ * // map is now a Map object with entries: { 'key1' => 'value1', 'key2' => 'value2' }
+ */
+export function deserializeArrayToMap(arr: [unknown, unknown][]) {


This is a very long name, and docstring. If someone is trying to understand this function, does this docstring tell them something useful that the implementation (new Map()) does not?

Suggested change

export function deserializeArrayToMap(arr: [unknown, unknown][]) {

export function tryNewMap(arr: [unknown, unknown][]) {

nkomonen-amazon · 2024-10-05T01:21:35Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+    constructor(private uri: vscode.Uri) {}
+
+    provideTextDocumentContent(_uri: vscode.Uri) {
+        return fs.readFileSync(this.uri.fsPath, 'utf-8')


Unless this needs to be synchronous, do not use fs-extra. The correct fs module is in our shared folder

Looking at the api, this method can be async. The type eventually resolves to a possible "Thenable". So you can use our built in fs module

nkomonen-amazon · 2024-10-05T01:32:22Z

packages/core/src/shared/utilities/textDocumentUtilities.ts


 import * as _path from 'path'
 import * as vscode from 'vscode'
+import fs from 'fs-extra'


avoid fs-extra if possible. Info in previous comment

nkomonen-amazon · 2024-10-07T14:22:34Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+                    })
+                )
+            } catch (error) {
+                getLogger().error(`Diff: Unable to open the document:`, (error as Error).message)


What is the behavior if we were to throw instead? If this code is initially triggered in a Command, then the error will be swallowed upstream, and logged.

Discussed offline.

Display an error Notification for the user. Update the logic to throw everywhere and catch at high level and display a generic error notification.

nkomonen-amazon · 2024-10-07T14:25:04Z

packages/core/src/shared/utilities/textDocumentUtilities.ts


 import * as _path from 'path'
 import * as vscode from 'vscode'
+import * as fs from 'fs'


This isn't the shared 'fs'. Use src/shared/fs/fs.ts

nkomonen-amazon · 2024-10-07T14:30:06Z

packages/core/src/shared/utilities/textDocumentUtilities.ts

+            fs.mkdirSync(tempDirPath, { recursive: true })
+        }
+    } catch (error) {
+        getLogger().error('Diff: Unable to create directory %s: %s', tempDirPath, (error as Error).message)


What is the reason for swallowing the errors? Especially in a utils function we shouldn't be swallowing, unless there is a good reason to. One reason is in telemetry when we fail and record the reason for the failure, we parse the entire error object for the chain of errors and then record it there. So if we only log then all that information is lost and requires the user to report it.

Instead you could make a function like this that wraps the error with context about the specific FS call that failed: https://github.com/aws/aws-toolkit-vscode/blob/master/packages/core/src/shared/crashMonitoring.ts#L589

Looking back at the I think that you should remove the try/catch and just call fs.mkdir() since its behavior is mkdirp() where it creates missing subfolders if necessary and does not throw if it already exists.

nkomonen-amazon · 2024-10-07T14:31:19Z

packages/core/src/shared/utilities/textDocumentUtilities.ts

+    const tempFile = _path.parse(originalFileUri.path)
+    const tempFilePath = _path.join(tempDirPath, `${tempFile.name}_proposed-${id}${tempFile.ext}`)
+    try {
+        if (!fs.existsSync(tempDirPath)) {


Avoid sync calls since they are blocking. See the shared fs mentioned above

nkomonen-amazon · 2024-10-07T18:19:31Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+                )
+            } catch (error) {
+                void vscode.window.showInformationMessage(errorNotification)
+                ChatDiffError.chain(error, `Failed to Accept Diff`, { code: chatDiffCode })


need to add throw

nkomonen-amazon · 2024-10-07T18:19:59Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+            }
+        } catch (error) {
+            void vscode.window.showInformationMessage(errorNotification)
+            ChatDiffError.chain(error, `Failed to Open Diff View`, { code: chatDiffCode })


need to add throw

nkomonen-amazon · 2024-10-07T18:27:49Z

packages/core/src/shared/utilities/textDocumentUtilities.ts

+        if (!(error instanceof Error)) {
+            throw error
+        }
+        getLogger().error('Diff: Unable to write to file %s: %s', tempFilePath, (error as Error).message)


Dont need to log the error since below we will throw the error and it will be logged upstream

nkomonen-amazon · 2024-10-07T18:28:45Z

packages/core/src/shared/utilities/textDocumentUtilities.ts

+            throw error
+        }
+        getLogger().error('Diff: Unable to write to file %s: %s', tempFilePath, (error as Error).message)
+        ToolkitError.chain(error, 'Unable to write to file', { code: errorCode })


add 'throw'. Also we want this error message to give context to why the FS call failed. So in this case it should be "Failed to write to temp file"

nkomonen-amazon · 2024-10-07T18:30:10Z

packages/core/src/amazonq/util/functionUtils.ts

+ * const map = tryNewMap(array);
+ * // map is now a Map object with entries: { 'key1' => 'value1', 'key2' => 'value2' }
+ */
+export function tryNewMap(arr: [unknown, unknown][]) {


This looks like it should live in core and since this is map related would probably make sense in core/src/shared/utilities/map

amazon/util doesn't look like the place for this. And, the docstring is still way too long and over-explains what could be stated as "tries to create map from x, else returns an empty map".

Also that pattern is easily generalized as tryCall<T>(...), so I don't see why we need a dedicated function for this.

nkomonen-amazon · 2024-10-10T15:09:53Z

packages/core/src/amazonq/commons/controllers/contentController.ts

+import { extractFileAndCodeSelectionFromMessage, fs, getErrorMsg, getIndentedCode, ToolkitError } from '../../../shared'
+
+class ContentProvider implements vscode.TextDocumentContentProvider {
+    constructor(private uri: vscode.Uri) {}


nit: private readonly uri: ... is typically preferred. But dont bother changing in this PR

justinmk3 · 2024-10-10T17:27:43Z

packages/core/src/shared/telemetry/vscodeTelemetry.json

                "downvote",
-                "clickBodyLink"
+                "clickBodyLink",
+                "viewDiff"


please upstream this: https://github.com/aws/aws-toolkit-common/blob/df10c59a61083162b6d97d2d4854a47debb01465/telemetry/definitions/commonDefinitions.json#L982

justinmk3 · 2024-10-10T17:32:20Z

packages/core/src/shared/utilities/textUtilities.ts

+ * @param {vscode.Selection} selection - The selection range in the document.
+ * @returns {string} - The processed code to be applied to the document.
+ */
+export function getIndentedCode(message: any, doc: vscode.TextDocument, selection: vscode.Selection) {


should this live in packages/core/src/shared/utilities/textDocumentUtilities.ts

ivikash requested review from a team as code owners July 18, 2024 05:45

justinmk3 reviewed Jul 22, 2024

View reviewed changes

justinmk3 approved these changes Jul 22, 2024

View reviewed changes

ivikash changed the title ~~feat(amazonq): add button to accept code changes in diff view~~ [Work In Progress] feat(amazonq): add button to accept code changes in diff view Jul 22, 2024

ivikash force-pushed the feat/diff-view branch from bb60012 to 58f5cd5 Compare September 19, 2024 15:25

ivikash force-pushed the feat/diff-view branch from 58f5cd5 to ccec026 Compare September 19, 2024 23:25

ivikash changed the title ~~[Work In Progress] feat(amazonq): add button to accept code changes in diff view~~ feat(amazonq): add button to accept code changes in diff view Sep 19, 2024

ivikash changed the title ~~feat(amazonq): add button to accept code changes in diff view~~ feat(amazonq): add button to view diff in IDE Sep 19, 2024

ivikash force-pushed the feat/diff-view branch 10 times, most recently from 0374d02 to 80ac383 Compare October 2, 2024 16:01

justinmk3 reviewed Oct 2, 2024

View reviewed changes

packages/core/src/amazonq/commons/diff.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Oct 2, 2024

View reviewed changes

packages/core/src/amazonq/commons/diff.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Oct 2, 2024

View reviewed changes

packages/core/src/amazonq/commons/diff.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Oct 2, 2024

View reviewed changes

packages/core/src/amazonq/commons/diff.ts Outdated Show resolved Hide resolved

justinmk3 reviewed Oct 2, 2024

View reviewed changes

ivikash force-pushed the feat/diff-view branch 2 times, most recently from ae5a75d to 051dd09 Compare October 2, 2024 18:44

ivikash force-pushed the feat/diff-view branch from 59d5a71 to a40bcb9 Compare October 4, 2024 04:26

justinmk3 reviewed Oct 4, 2024

View reviewed changes

justinmk3 approved these changes Oct 4, 2024

View reviewed changes

nkomonen-amazon reviewed Oct 5, 2024

View reviewed changes

ivikash force-pushed the feat/diff-view branch 2 times, most recently from b58d1f7 to e0d7464 Compare October 6, 2024 20:23

nkomonen-amazon reviewed Oct 7, 2024

View reviewed changes

ivikash force-pushed the feat/diff-view branch 2 times, most recently from 615332d to bf8f4dd Compare October 7, 2024 17:39

nkomonen-amazon reviewed Oct 7, 2024

View reviewed changes

ivikash force-pushed the feat/diff-view branch 2 times, most recently from 155042e to c669d06 Compare October 10, 2024 03:04

nkomonen-amazon reviewed Oct 10, 2024

View reviewed changes

nkomonen-amazon approved these changes Oct 10, 2024

View reviewed changes

feat(amazonq): add button to view and accept diff

63b2ed8

ivikash force-pushed the feat/diff-view branch from c669d06 to 63b2ed8 Compare October 10, 2024 16:50

ivikash merged commit 62e9cfb into aws:master Oct 10, 2024
21 of 24 checks passed

ivikash deleted the feat/diff-view branch October 10, 2024 17:20

justinmk3 reviewed Oct 10, 2024

View reviewed changes

	* URI scheme for CloudWatch Logs Virtual Documents
	*/
	export const CLOUDWATCH_LOGS_SCHEME = 'aws-cwl' // eslint-disable-line @typescript-eslint/naming-convention
	export const AWS_SCHEME = 'aws' // eslint-disable-line @typescript-eslint/naming-convention

	export const AMAZON_Q_DIFF_SCHEME = 'amazon-q-view-diff' // eslint-disable-line @typescript-eslint/naming-convention
	export const amazonQDiffScheme = 'amazon-q-diff'

	export function deserializeArrayToMap(arr: [unknown, unknown][]) {
	export function tryNewMap(arr: [unknown, unknown][]) {

feat(amazonq): add button to view diff in IDE #5338

feat(amazonq): add button to view diff in IDE #5338

Uh oh!

Conversation

ivikash commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk3 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 19, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivikash Oct 2, 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!

Choose a reason for hiding this comment

Uh oh!

justinmk3 Oct 4, 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!

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!

nkomonen-amazon Oct 7, 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!

nkomonen-amazon Oct 10, 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!

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!

justinmk3 Oct 10, 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!

Uh oh!

ivikash commented Jul 18, 2024 •

edited

Loading

ivikash Oct 2, 2024 •

edited

Loading

justinmk3 Oct 4, 2024 •

edited

Loading

nkomonen-amazon Oct 7, 2024 •

edited

Loading

nkomonen-amazon Oct 10, 2024 •

edited

Loading

justinmk3 Oct 10, 2024 •

edited

Loading