-
Notifications
You must be signed in to change notification settings - Fork 749
fix(chat): use diff lib to get diffs #6940
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,9 @@ import { getLogger } from '../../shared/logger/logger' | |
| import vscode from 'vscode' | ||
| import { fs } from '../../shared/fs/fs' | ||
| import { Writable } from 'stream' | ||
| import { Change, diffLines } from 'diff' | ||
| import { getDiffMarkdown } from '../../shared/utilities/diffUtils' | ||
| import { getLanguageForFilePath } from '../../shared/utilities/textDocumentUtilities' | ||
|
|
||
| interface BaseParams { | ||
| path: string | ||
|
|
@@ -69,114 +72,40 @@ export class FsWrite { | |
| } | ||
| } | ||
|
|
||
| // TODO: Refactor the fsWrite.ts file "queueDescription" method to use existing diffLines and diff library to get the diff preview. or reuse existing diff view logic in cwchat. This will be part of next PR. | ||
| private showStrReplacePreview(oldStr: string, newStr: string): string { | ||
| // Split both strings into arrays of lines | ||
| const oldStrLines = oldStr.split('\n') | ||
| const newStrLines = newStr.split('\n') | ||
| let result = '' | ||
|
|
||
| // If strings are identical, return empty string | ||
| if (oldStr === newStr) { | ||
| return result | ||
| } | ||
|
|
||
| let oldLineIndex = 0 | ||
| let newLineIndex = 0 | ||
| // Loop through both arrays until we've processed all lines | ||
| while (oldLineIndex < oldStrLines.length || newLineIndex < newStrLines.length) { | ||
| if ( | ||
| oldLineIndex < oldStrLines.length && | ||
| newLineIndex < newStrLines.length && | ||
| oldStrLines[oldLineIndex] === newStrLines[newLineIndex] | ||
| ) { | ||
| // Line is unchanged - prefix with space | ||
| result += ` ${oldStrLines[oldLineIndex]}\n` | ||
| oldLineIndex++ | ||
| newLineIndex++ | ||
| } else { | ||
| // Line is different | ||
| if (oldLineIndex < oldStrLines.length) { | ||
| // Remove line - prefix with minus | ||
| result += `- ${oldStrLines[oldLineIndex]}\n` | ||
| oldLineIndex++ | ||
| } | ||
| if (newLineIndex < newStrLines.length) { | ||
| // Add line - prefix with plus | ||
| result += `+ ${newStrLines[newLineIndex]}\n` | ||
| newLineIndex++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| private async showInsertPreview(path: string, insertLine: number, newStr: string): Promise<string> { | ||
| const fileContent = await fs.readFileText(path) | ||
| const lines = fileContent.split('\n') | ||
| const startLine = Math.max(0, insertLine - 2) | ||
| const endLine = Math.min(lines.length, insertLine + 3) | ||
|
|
||
| const contextLines: string[] = [] | ||
|
|
||
| // Add lines before insertion point | ||
| for (let index = startLine; index < insertLine; index++) { | ||
| contextLines.push(` ${lines[index]}`) | ||
| } | ||
|
|
||
| // Add the new line with a '+' prefix | ||
| contextLines.push(`+ ${newStr}`) | ||
|
|
||
| // Add lines after insertion point | ||
| for (let index = insertLine; index < endLine; index++) { | ||
| contextLines.push(` ${lines[index]}`) | ||
| } | ||
| public async queueDescription(updates: Writable): Promise<void> { | ||
| const sanitizedPath = sanitizePath(this.params.path) | ||
| const changes = await this.getDiffChanges() | ||
| const language = await getLanguageForFilePath(sanitizedPath) | ||
|
|
||
| return contextLines.join('\n') | ||
| const diff = getDiffMarkdown(changes, language) | ||
| updates.write(diff) | ||
| updates.end() | ||
| } | ||
|
|
||
| private async showAppendPreview(sanitizedPath: string, newStr: string) { | ||
| const fileContent = await fs.readFileText(sanitizedPath) | ||
| const needsNewline = fileContent.length !== 0 && !fileContent.endsWith('\n') | ||
|
|
||
| let contentToAppend = newStr | ||
| if (needsNewline) { | ||
| contentToAppend = '\n' + contentToAppend | ||
| public async getDiffChanges(): Promise<Change[]> { | ||
| const sanitizedPath = sanitizePath(this.params.path) | ||
| let newContent | ||
| let oldContent | ||
| try { | ||
| oldContent = await fs.readFileText(sanitizedPath) | ||
| } catch (err) { | ||
| oldContent = '' | ||
| } | ||
|
|
||
| // Get the last 3 lines from existing content for better UX | ||
| const lines = fileContent.split('\n') | ||
| const linesForContext = lines.slice(-3) | ||
|
|
||
| return `${linesForContext.join('\n')}\n+ ${contentToAppend.trim()}` | ||
| } | ||
|
|
||
| public async queueDescription(updates: Writable): Promise<void> { | ||
| switch (this.params.command) { | ||
| case 'create': | ||
| updates.write(`\`\`\`diff-typescript | ||
| ${'+' + this.params.fileText?.replace(/\n/g, '\n+')} | ||
| `) | ||
| newContent = this.getCreateCommandText(this.params) | ||
| break | ||
| case 'strReplace': | ||
| updates.write(`\`\`\`diff-typescript | ||
| ${this.showStrReplacePreview(this.params.oldStr, this.params.newStr)} | ||
| \`\`\` | ||
| `) | ||
| newContent = await this.getStrReplaceContent(this.params, sanitizedPath) | ||
| break | ||
| case 'insert': | ||
| updates.write(`\`\`\`diff-typescript | ||
| ${await this.showInsertPreview(this.params.path, this.params.insertLine, this.params.newStr)} | ||
| \`\`\``) | ||
| newContent = await this.getInsertContent(this.params, sanitizedPath) | ||
| break | ||
| case 'append': | ||
| updates.write(`\`\`\`diff-typescript | ||
| ${await this.showAppendPreview(this.params.path, this.params.newStr)} | ||
| \`\`\``) | ||
| newContent = await this.getAppendContent(this.params, sanitizedPath) | ||
| break | ||
| } | ||
| updates.end() | ||
| return diffLines(oldContent, newContent) | ||
| } | ||
|
|
||
| public async validate(): Promise<void> { | ||
|
|
@@ -224,6 +153,11 @@ ${await this.showAppendPreview(this.params.path, this.params.newStr)} | |
| } | ||
|
|
||
| private async handleStrReplace(params: StrReplaceParams, sanitizedPath: string): Promise<void> { | ||
| const newContent = await this.getStrReplaceContent(params, sanitizedPath) | ||
| await fs.writeFile(sanitizedPath, newContent) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does caller has error handling for these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes any error thrown from a tool will get sent back to the LLM |
||
| } | ||
|
|
||
| private async getStrReplaceContent(params: StrReplaceParams, sanitizedPath: string): Promise<string> { | ||
| const fileContent = await fs.readFileText(sanitizedPath) | ||
|
|
||
| const matches = [...fileContent.matchAll(new RegExp(this.escapeRegExp(params.oldStr), 'g'))] | ||
|
|
@@ -235,11 +169,15 @@ ${await this.showAppendPreview(this.params.path, this.params.newStr)} | |
| throw new Error(`${matches.length} occurrences of oldStr were found when only 1 is expected`) | ||
| } | ||
|
|
||
| const newContent = fileContent.replace(params.oldStr, params.newStr) | ||
| await fs.writeFile(sanitizedPath, newContent) | ||
| return fileContent.replace(params.oldStr, params.newStr) | ||
| } | ||
|
|
||
| private async handleInsert(params: InsertParams, sanitizedPath: string): Promise<void> { | ||
| const newContent = await this.getInsertContent(params, sanitizedPath) | ||
| await fs.writeFile(sanitizedPath, newContent) | ||
| } | ||
|
|
||
| private async getInsertContent(params: InsertParams, sanitizedPath: string): Promise<string> { | ||
| const fileContent = await fs.readFileText(sanitizedPath) | ||
| const lines = fileContent.split('\n') | ||
|
|
||
|
|
@@ -252,11 +190,15 @@ ${await this.showAppendPreview(this.params.path, this.params.newStr)} | |
| } else { | ||
| newContent = [...lines.slice(0, insertLine), params.newStr, ...lines.slice(insertLine)].join('\n') | ||
| } | ||
| return newContent | ||
| } | ||
|
|
||
| private async handleAppend(params: AppendParams, sanitizedPath: string): Promise<void> { | ||
| const newContent = await this.getAppendContent(params, sanitizedPath) | ||
| await fs.writeFile(sanitizedPath, newContent) | ||
| } | ||
|
|
||
| private async handleAppend(params: AppendParams, sanitizedPath: string): Promise<void> { | ||
| private async getAppendContent(params: AppendParams, sanitizedPath: string): Promise<string> { | ||
| const fileContent = await fs.readFileText(sanitizedPath) | ||
| const needsNewline = fileContent.length !== 0 && !fileContent.endsWith('\n') | ||
|
|
||
|
|
@@ -265,8 +207,7 @@ ${await this.showAppendPreview(this.params.path, this.params.newStr)} | |
| contentToAppend = '\n' + contentToAppend | ||
| } | ||
|
|
||
| const newContent = fileContent + contentToAppend | ||
| await fs.writeFile(sanitizedPath, newContent) | ||
| return fileContent + contentToAppend | ||
| } | ||
|
|
||
| private getCreateCommandText(params: CreateParams): string { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,6 +224,27 @@ export async function addEofNewline(editor: vscode.TextEditor) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Determines the language identifier for a file at the specified path. | ||
| * This function attempts to open the file as a VS Code document to retrieve its language ID. | ||
| * | ||
| * @param {string} filePath - The absolute or relative path to the file whose language should be determined. | ||
| * @returns {Promise<string | undefined>} A promise that resolves to the language identifier of the file, | ||
| * or undefined if the file cannot be opened or the language cannot be determined. | ||
| * @example | ||
| * // Get language for a JavaScript file | ||
| * const language = await getLanguageForFilePath('/path/to/file.js'); | ||
| * // language will be 'javascript' | ||
| */ | ||
| export async function getLanguageForFilePath(filePath: string): Promise<string | undefined> { | ||
| try { | ||
| const document = await vscode.workspace.openTextDocument(filePath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this one recognize the languages that's not supported by VS Code natively?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think right now no, the language will have to be recognized by vscode
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did product ack this? or we can switch to use the file extension matching logic that we have
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only recognize languages natively supported by Visual Studio Code, though it covers most programming languages. For unsupported languages, it defaults to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that, my question is if this is what product/UX expected, because some of them are technically supported by Q although not by VS Code
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we use our file extension matching logic it might be too restrictive? For new languages we have to always update the map. Another reason I was thinking to use VSCode API is that the markdown diff view and the native diff view should have consistent syntax highlighting
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is ok to follow up |
||
| return document.languageId | ||
| } catch (err) { | ||
| return | ||
| } | ||
| } | ||
|
|
||
| class ReadonlyTextDocumentProvider implements vscode.TextDocumentContentProvider { | ||
| private content = '' | ||
|
|
||
|
|
||
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.
should we log it? what is the expected UX if we cannot read the file, do we still want to proceed with showing diffs?
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.
LLM sends the filePath which may or may not exist
If fileExists we take the oldContent if not we consider this as a new file and keep oldContent as ''.
I think we should not inform end customer about this!
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.
Yeah in the
createcommand case, this is expected to fail because the file doesn't exist yet. So I don't think its necessary to show an error