From aa789245f4c9c5013c04a7022528409a707dc929 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Thu, 5 Jun 2025 17:33:51 -0700 Subject: [PATCH 1/5] 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 9d4ab074f69a217ec75827544a178ae6f7aaf008 Mon Sep 17 00:00:00 2001 From: Vario Date: Sat, 7 Jun 2025 16:01:46 -0700 Subject: [PATCH 2/5] 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 9cbe3bb922848257eb2d08bd1efd4a415b92c2bb Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Sat, 7 Jun 2025 16:13:52 -0700 Subject: [PATCH 3/5] 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 c51dc1795800724fee028dbd15fdb7ac7b7d0bf7 Mon Sep 17 00:00:00 2001 From: Mnehmos Date: Tue, 10 Jun 2025 12:29:31 -0700 Subject: [PATCH 4/5] 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) From 9e559b849044f4903d849d9faa12f41140a4402e Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Fri, 13 Jun 2025 12:52:50 -0500 Subject: [PATCH 5/5] fix: improve error handling and timeout management in DiffViewProvider --- src/integrations/editor/DiffViewProvider.ts | 153 ++++++++++---------- 1 file changed, 76 insertions(+), 77 deletions(-) diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index 61ea45edd29..b97886d32dc 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -394,7 +394,7 @@ export class DiffViewProvider { vscode.window.tabGroups.close(tab).then( () => undefined, (err) => { - // Ignore errors when closing diff tabs - they may already be closed + console.error(`Failed to close diff tab ${tab.label}`, err) }, ), ) @@ -404,7 +404,9 @@ export class DiffViewProvider { private async openDiffEditor(): Promise { if (!this.relPath) { - throw new Error("No file path set") + throw new Error( + "No file path set for opening diff editor. Ensure open() was called before openDiffEditor()", + ) } const uri = vscode.Uri.file(path.resolve(this.cwd, this.relPath)) @@ -428,86 +430,83 @@ export class DiffViewProvider { // Open new diff editor. return new Promise((resolve, reject) => { - ;(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 - } + const fileName = path.basename(uri.fsPath) + const fileExists = this.editType === "modify" + const DIFF_EDITOR_TIMEOUT = 10_000 // ms + + let timeoutId: NodeJS.Timeout | undefined + const disposables: vscode.Disposable[] = [] + + const cleanup = () => { + if (timeoutId) { + clearTimeout(timeoutId) + timeoutId = undefined + } + disposables.forEach((d) => d.dispose()) + disposables.length = 0 + } + + // Set timeout for the entire operation + timeoutId = setTimeout(() => { + cleanup() + reject( + new Error( + `Failed to open diff editor for ${uri.fsPath} within ${DIFF_EDITOR_TIMEOUT / 1000} seconds. The editor may be blocked or VS Code may be unresponsive.`, + ), + ) + }, DIFF_EDITOR_TIMEOUT) + + // Listen for document open events - more efficient than scanning all tabs + disposables.push( + vscode.workspace.onDidOpenTextDocument(async (document) => { + if (arePathsEqual(document.uri.fsPath, uri.fsPath)) { + // Wait a tick for the editor to be available + await new Promise((r) => setTimeout(r, 0)) + + // Find the editor for this document + const editor = vscode.window.visibleTextEditors.find((e) => + arePathsEqual(e.document.uri.fsPath, uri.fsPath), + ) + + if (editor) { + cleanup() + resolve(editor) } } - return false - } + }), + ) - // Listen for changes in tab groups, which includes tabs moving between windows - const disposableTabGroup = vscode.window.tabGroups.onDidChangeTabGroups(() => { - const found = checkAndResolve() - if (found) { - // Editor found and resolved, no need to continue listening - console.debug("Diff editor found via tab group change listener") + // Also listen for visible editor changes as a fallback + disposables.push( + vscode.window.onDidChangeVisibleTextEditors((editors) => { + const editor = editors.find((e) => arePathsEqual(e.document.uri.fsPath, uri.fsPath)) + if (editor) { + cleanup() + resolve(editor) } - }) + }), + ) - 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( - () => { - // Give a brief moment for the editor to appear in tab groups - 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) - 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) - })() + // Execute the diff command + 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( + () => { + // Command executed successfully, now wait for the editor to appear + }, + (err: any) => { + cleanup() + reject(new Error(`Failed to execute diff command for ${uri.fsPath}: ${err.message}`)) + }, + ) }) }