From 2321243583d71fa54d9d09e88bfc00cf87feddd6 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Thu, 5 Jun 2025 17:33:51 -0700 Subject: [PATCH 1/4] fix(4385): re-apply changes from PR 4385 --- src/integrations/editor/DiffViewProvider.ts | 98 ++++++++++++++------- 1 file changed, 68 insertions(+), 30 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 3ab0419618f..fe9a123a035 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -349,10 +349,7 @@ export class DiffViewProvider { // Remove only the directories we created, in reverse order. for (let i = this.createdDirs.length - 1; i >= 0; i--) { await fs.rmdir(this.createdDirs[i]) - console.log(`Directory ${this.createdDirs[i]} has been deleted.`) } - - console.log(`File ${absolutePath} has been deleted.`) } else { // Revert document. const edit = new vscode.WorkspaceEdit() @@ -369,7 +366,6 @@ export class DiffViewProvider { // changes and saved during the edit. await vscode.workspace.applyEdit(edit) await updatedDocument.save() - console.log(`File ${absolutePath} has been reverted to its original content.`) if (this.documentWasOpen) { await vscode.window.showTextDocument(vscode.Uri.file(absolutePath), { @@ -397,9 +393,7 @@ export class DiffViewProvider { .map((tab) => vscode.window.tabGroups.close(tab).then( () => undefined, - (err) => { - console.error(`Failed to close diff tab ${tab.label}`, err) - }, + (err) => {}, ), ) @@ -432,31 +426,75 @@ export class DiffViewProvider { // Open new diff editor. return new Promise((resolve, reject) => { - const fileName = path.basename(uri.fsPath) - const fileExists = this.editType === "modify" - - const disposable = vscode.window.onDidChangeActiveTextEditor((editor) => { - if (editor && arePathsEqual(editor.document.uri.fsPath, uri.fsPath)) { - disposable.dispose() - resolve(editor) + ;(async () => { + const fileName = path.basename(uri.fsPath) + const fileExists = this.editType === "modify" + let timeoutId: NodeJS.Timeout | undefined + + const checkAndResolve = () => { + for (const group of vscode.window.tabGroups.all) { + for (const tab of group.tabs) { + if ( + tab.input instanceof vscode.TabInputTextDiff && + tab.input?.original?.scheme === DIFF_VIEW_URI_SCHEME && + arePathsEqual(tab.input.modified.fsPath, uri.fsPath) + ) { + // Found the diff editor, now try to show it to get the TextEditor instance + vscode.window.showTextDocument(tab.input.modified, { preserveFocus: true }).then( + (editor) => { + if (timeoutId) clearTimeout(timeoutId) + disposableTabGroup.dispose() + resolve(editor) + }, + (err) => { + if (timeoutId) clearTimeout(timeoutId) + disposableTabGroup.dispose() + reject( + new Error(`Failed to show diff editor after finding tab: ${err.message}`), + ) + }, + ) + return true + } + } + } + return false } - }) - - vscode.commands.executeCommand( - "vscode.diff", - vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({ - query: Buffer.from(this.originalContent ?? "").toString("base64"), - }), - uri, - `${fileName}: ${fileExists ? "Original ↔ Roo's Changes" : "New File"} (Editable)`, - { preserveFocus: true }, - ) - // This may happen on very slow machines i.e. project idx. - setTimeout(() => { - disposable.dispose() - reject(new Error("Failed to open diff editor, please try again...")) - }, 10_000) + // Listen for changes in tab groups, which includes tabs moving between windows + const disposableTabGroup = vscode.window.tabGroups.onDidChangeTabGroups(() => { + checkAndResolve() + }) + + vscode.commands + .executeCommand( + "vscode.diff", + vscode.Uri.parse(`${DIFF_VIEW_URI_SCHEME}:${fileName}`).with({ + query: Buffer.from(this.originalContent ?? "").toString("base64"), + }), + uri, + `${fileName}: ${fileExists ? "Original ↔ Roo's Changes" : "New File"} (Editable)`, + { preserveFocus: true }, + ) + .then( + async () => { + // Give a brief moment for the editor to appear in tab groups + await new Promise((r) => setTimeout(r, 100)) + if (!checkAndResolve()) { + } + }, + (err) => { + if (timeoutId) clearTimeout(timeoutId) + disposableTabGroup.dispose() + reject(new Error(`Failed to open diff editor command: ${err.message}`)) + }, + ) + + timeoutId = setTimeout(() => { + disposableTabGroup.dispose() + reject(new Error("Failed to open diff editor, please try again...")) + }, 10_000) + })() }) } From 038d975dee4fbe657dd766f8aea80cc8b13ebe16 Mon Sep 17 00:00:00 2001 From: Vario Date: Sat, 7 Jun 2025 16:01:46 -0700 Subject: [PATCH 2/4] Update src/integrations/editor/DiffViewProvider.ts Co-authored-by: Daniel <57051444+daniel-lxs@users.noreply.github.com> --- src/integrations/editor/DiffViewProvider.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index fe9a123a035..7281b71d6a0 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -393,7 +393,9 @@ export class DiffViewProvider { .map((tab) => vscode.window.tabGroups.close(tab).then( () => undefined, - (err) => {}, +(err) => { + // Ignore errors when closing diff tabs - they may already be closed +}, ), ) From d36dc4d9ddc98463f058fec115498b9e6bbae509 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Sat, 7 Jun 2025 16:13:52 -0700 Subject: [PATCH 3/4] Update DiffViewProvider.ts No async / await: We don't need to await the timeout. We just want to schedule checkAndResolve to run in the future, not pause the entire function. setTimeout does this perfectly on its own. No if Statement: The confusing if statement is gone. We are no longer checking the return value because we don't need to do anything with it in this specific spot. The primary purpose here is just to trigger the check. Clear Intent: The corrected code is much clearer. Its only job is to kick off the checkAndResolve function after 100ms. This acts as a simple, manual trigger to supplement the main onDidChangeTabGroups event listener, which is the more robust part of the fix. --- src/integrations/editor/DiffViewProvider.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 7281b71d6a0..bcfecb2a6c1 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -393,9 +393,9 @@ export class DiffViewProvider { .map((tab) => vscode.window.tabGroups.close(tab).then( () => undefined, -(err) => { - // Ignore errors when closing diff tabs - they may already be closed -}, + (err) => { + // Ignore errors when closing diff tabs - they may already be closed + }, ), ) @@ -479,11 +479,9 @@ export class DiffViewProvider { { preserveFocus: true }, ) .then( - async () => { + () => { // Give a brief moment for the editor to appear in tab groups - await new Promise((r) => setTimeout(r, 100)) - if (!checkAndResolve()) { - } + setTimeout(checkAndResolve, 100) }, (err) => { if (timeoutId) clearTimeout(timeoutId) From 6b78be78e9abd7b1ba73f2ee36aecfa9fcfba0b5 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Tue, 10 Jun 2025 12:29:31 -0700 Subject: [PATCH 4/4] Fix incomplete conditional block in DiffViewProvider.ts - Add proper handling for checkAndResolve() return value in setTimeout callback - Enhance tab group change listener to handle boolean return value appropriately - Add debug logging for better troubleshooting of diff editor opening issues - Maintain all existing functionality while addressing code review feedback Addresses review feedback from Daniel in PR #4394 regarding incomplete conditional handling around line 484. --- src/integrations/editor/DiffViewProvider.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index bcfecb2a6c1..61ea45edd29 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -465,7 +465,11 @@ export class DiffViewProvider { // Listen for changes in tab groups, which includes tabs moving between windows const disposableTabGroup = vscode.window.tabGroups.onDidChangeTabGroups(() => { - checkAndResolve() + const found = checkAndResolve() + if (found) { + // Editor found and resolved, no need to continue listening + console.debug("Diff editor found via tab group change listener") + } }) vscode.commands @@ -481,7 +485,16 @@ export class DiffViewProvider { .then( () => { // Give a brief moment for the editor to appear in tab groups - setTimeout(checkAndResolve, 100) + setTimeout(() => { + const found = checkAndResolve() + if (!found) { + // If not found immediately, rely on tab group change listener + // and the 10-second timeout fallback to handle resolution + console.debug( + "Diff editor not found in initial check, waiting for tab group changes...", + ) + } + }, 100) }, (err) => { if (timeoutId) clearTimeout(timeoutId)