From 6b68b164cfae1dd0e0ba192b03076a9c15bace30 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 19 Jul 2025 18:42:44 +0000 Subject: [PATCH] fix: prevent "TextEditor is closed/disposed" warnings - Add disposal tracking and checks to DecorationController - Add try-catch blocks around TextEditor property access - Properly dispose decoration controllers in DiffViewProvider - Add comprehensive tests for disposal handling - Protect editor access in getEnvironmentDetails and registerCommands Fixes #5954 --- src/activate/registerCommands.ts | 11 +- src/core/environment/getEnvironmentDetails.ts | 11 +- .../editor/DecorationController.ts | 90 ++++++- src/integrations/editor/DiffViewProvider.ts | 100 +++++--- .../__tests__/DecorationController.spec.ts | 234 ++++++++++++++++++ 5 files changed, 406 insertions(+), 40 deletions(-) create mode 100644 src/integrations/editor/__tests__/DecorationController.spec.ts diff --git a/src/activate/registerCommands.ts b/src/activate/registerCommands.ts index bd925b0e90..fe9d4112d0 100644 --- a/src/activate/registerCommands.ts +++ b/src/activate/registerCommands.ts @@ -238,7 +238,16 @@ export const openClineInNewTab = async ({ context, outputChannel }: Omit editor.viewColumn || 0)) + const lastCol = Math.max( + ...vscode.window.visibleTextEditors.map((editor) => { + try { + return editor.viewColumn || 0 + } catch { + // Editor might be disposed + return 0 + } + }), + ) // Check if there are any visible text editors, otherwise open a new group // to the right. diff --git a/src/core/environment/getEnvironmentDetails.ts b/src/core/environment/getEnvironmentDetails.ts index b83b37c75b..2e896eaa96 100644 --- a/src/core/environment/getEnvironmentDetails.ts +++ b/src/core/environment/getEnvironmentDetails.ts @@ -37,8 +37,15 @@ export async function getEnvironmentDetails(cline: Task, includeFileDetails: boo details += "\n\n# VSCode Visible Files" const visibleFilePaths = vscode.window.visibleTextEditors - ?.map((editor) => editor.document?.uri?.fsPath) - .filter(Boolean) + ?.map((editor) => { + try { + return editor.document?.uri?.fsPath + } catch { + // Editor might be disposed + return null + } + }) + .filter((fsPath): fsPath is string => fsPath !== null && fsPath !== undefined) .map((absolutePath) => path.relative(cline.cwd, absolutePath)) .slice(0, maxWorkspaceFiles) diff --git a/src/integrations/editor/DecorationController.ts b/src/integrations/editor/DecorationController.ts index 8f475408d4..66d4c3da2b 100644 --- a/src/integrations/editor/DecorationController.ts +++ b/src/integrations/editor/DecorationController.ts @@ -19,6 +19,7 @@ export class DecorationController { private decorationType: DecorationType private editor: vscode.TextEditor private ranges: vscode.Range[] = [] + private isDisposed: boolean = false constructor(decorationType: DecorationType, editor: vscode.TextEditor) { this.decorationType = decorationType @@ -35,8 +36,13 @@ export class DecorationController { } addLines(startIndex: number, numLines: number) { - // Guard against invalid inputs - if (startIndex < 0 || numLines <= 0) { + // Guard against invalid inputs or disposed state + if (startIndex < 0 || numLines <= 0 || this.isDisposed) { + return + } + + // Check if editor is still valid before using it + if (!this.isEditorValid()) { return } @@ -48,15 +54,43 @@ export class DecorationController { this.ranges.push(new vscode.Range(startIndex, 0, endLine, Number.MAX_SAFE_INTEGER)) } - this.editor.setDecorations(this.getDecoration(), this.ranges) + try { + this.editor.setDecorations(this.getDecoration(), this.ranges) + } catch (error) { + // Editor was disposed between check and use + console.debug("DecorationController: Failed to set decorations, editor may be disposed", error) + } } clear() { + if (this.isDisposed) { + return + } + this.ranges = [] - this.editor.setDecorations(this.getDecoration(), this.ranges) + + if (!this.isEditorValid()) { + return + } + + try { + this.editor.setDecorations(this.getDecoration(), this.ranges) + } catch (error) { + // Editor was disposed between check and use + console.debug("DecorationController: Failed to clear decorations, editor may be disposed", error) + } } updateOverlayAfterLine(line: number, totalLines: number) { + if (this.isDisposed) { + return + } + + // Check if editor is still valid before using it + if (!this.isEditorValid()) { + return + } + // Remove any existing ranges that start at or after the current line this.ranges = this.ranges.filter((range) => range.end.line < line) @@ -71,11 +105,55 @@ export class DecorationController { } // Apply the updated decorations - this.editor.setDecorations(this.getDecoration(), this.ranges) + try { + this.editor.setDecorations(this.getDecoration(), this.ranges) + } catch (error) { + // Editor was disposed between check and use + console.debug("DecorationController: Failed to update overlay, editor may be disposed", error) + } } setActiveLine(line: number) { + if (this.isDisposed) { + return + } + + // Check if editor is still valid before using it + if (!this.isEditorValid()) { + return + } + this.ranges = [new vscode.Range(line, 0, line, Number.MAX_SAFE_INTEGER)] - this.editor.setDecorations(this.getDecoration(), this.ranges) + + try { + this.editor.setDecorations(this.getDecoration(), this.ranges) + } catch (error) { + // Editor was disposed between check and use + console.debug("DecorationController: Failed to set active line, editor may be disposed", error) + } + } + + /** + * Checks if the editor is still valid and not disposed + */ + private isEditorValid(): boolean { + try { + // Try to access a property that would throw if disposed + // The document property is a good indicator + const _ = this.editor.document + return true + } catch { + // Editor is disposed + this.isDisposed = true + return false + } + } + + /** + * Marks this controller as disposed + */ + dispose() { + this.isDisposed = true + this.clear() } } diff --git a/src/integrations/editor/DiffViewProvider.ts b/src/integrations/editor/DiffViewProvider.ts index f4133029c9..c50c0f814b 100644 --- a/src/integrations/editor/DiffViewProvider.ts +++ b/src/integrations/editor/DiffViewProvider.ts @@ -99,8 +99,12 @@ export class DiffViewProvider { this.fadedOverlayController = new DecorationController("fadedOverlay", this.activeDiffEditor) this.activeLineController = new DecorationController("activeLine", this.activeDiffEditor) // Apply faded overlay to all lines initially. - this.fadedOverlayController.addLines(0, this.activeDiffEditor.document.lineCount) - this.scrollEditorToLine(0) // Will this crash for new files? + try { + this.fadedOverlayController.addLines(0, this.activeDiffEditor.document.lineCount) + this.scrollEditorToLine(0) + } catch (error) { + console.debug("DiffViewProvider: Failed to initialize decorations", error) + } this.streamedLines = [] } @@ -117,7 +121,14 @@ export class DiffViewProvider { } const diffEditor = this.activeDiffEditor - const document = diffEditor?.document + + // Check if editor is still valid + let document: vscode.TextDocument | undefined + try { + document = diffEditor?.document + } catch { + throw new Error("Text editor is disposed, unable to edit file...") + } if (!diffEditor || !document) { throw new Error("User closed text editor, unable to edit file...") @@ -125,8 +136,13 @@ export class DiffViewProvider { // Place cursor at the beginning of the diff editor to keep it out of // the way of the stream animation, but do this without stealing focus - const beginningOfDocument = new vscode.Position(0, 0) - diffEditor.selection = new vscode.Selection(beginningOfDocument, beginningOfDocument) + try { + const beginningOfDocument = new vscode.Position(0, 0) + diffEditor.selection = new vscode.Selection(beginningOfDocument, beginningOfDocument) + } catch (error) { + // Editor might be disposed, continue with the update + console.debug("DiffViewProvider: Failed to set selection", error) + } const endLine = accumulatedLines.length // Replace all content up to the current line with accumulated lines. @@ -176,12 +192,15 @@ export class DiffViewProvider { await vscode.workspace.applyEdit(finalEdit) // Clear all decorations at the end (after applying final edit). - this.fadedOverlayController.clear() - this.activeLineController.clear() + this.fadedOverlayController?.clear() + this.activeLineController?.clear() } } - async saveChanges(diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS): Promise<{ + async saveChanges( + diagnosticsEnabled: boolean = true, + writeDelayMs: number = DEFAULT_WRITE_DELAY_MS, + ): Promise<{ newProblemsMessage: string | undefined userEdits: string | undefined finalContent: string | undefined @@ -216,22 +235,22 @@ export class DiffViewProvider { // and can address them accordingly. If problems don't change immediately after // applying a fix, won't be notified, which is generally fine since the // initial fix is usually correct and it may just take time for linters to catch up. - + let newProblemsMessage = "" - + if (diagnosticsEnabled) { // Add configurable delay to allow linters time to process and clean up issues // like unused imports (especially important for Go and other languages) // Ensure delay is non-negative const safeDelayMs = Math.max(0, writeDelayMs) - + try { await delay(safeDelayMs) } catch (error) { // Log error but continue - delay failure shouldn't break the save operation console.warn(`Failed to apply write delay: ${error}`) } - + const postDiagnostics = vscode.languages.getDiagnostics() const newProblems = await diagnosticsToProblemsString( @@ -549,13 +568,19 @@ export class DiffViewProvider { } private scrollEditorToLine(line: number) { - if (this.activeDiffEditor) { - const scrollLine = line + 4 + if (!this.activeDiffEditor) { + return + } + try { + const scrollLine = line + 4 this.activeDiffEditor.revealRange( new vscode.Range(scrollLine, 0, scrollLine, 0), vscode.TextEditorRevealType.InCenter, ) + } catch (error) { + // Editor might be disposed + console.debug("DiffViewProvider: Failed to scroll editor", error) } } @@ -564,25 +589,30 @@ export class DiffViewProvider { return } - const currentContent = this.activeDiffEditor.document.getText() - const diffs = diff.diffLines(this.originalContent || "", currentContent) + try { + const currentContent = this.activeDiffEditor.document.getText() + const diffs = diff.diffLines(this.originalContent || "", currentContent) - let lineCount = 0 + let lineCount = 0 - for (const part of diffs) { - if (part.added || part.removed) { - // Found the first diff, scroll to it without stealing focus. - this.activeDiffEditor.revealRange( - new vscode.Range(lineCount, 0, lineCount, 0), - vscode.TextEditorRevealType.InCenter, - ) + for (const part of diffs) { + if (part.added || part.removed) { + // Found the first diff, scroll to it without stealing focus. + this.activeDiffEditor.revealRange( + new vscode.Range(lineCount, 0, lineCount, 0), + vscode.TextEditorRevealType.InCenter, + ) - return - } - - if (!part.removed) { - lineCount += part.count || 0 + return + } + + if (!part.removed) { + lineCount += part.count || 0 + } } + } catch (error) { + // Editor might be disposed + console.debug("DiffViewProvider: Failed to scroll to first diff", error) } } @@ -599,6 +629,16 @@ export class DiffViewProvider { } async reset(): Promise { + // Dispose decoration controllers before clearing references + if (this.fadedOverlayController) { + this.fadedOverlayController.dispose() + this.fadedOverlayController = undefined + } + if (this.activeLineController) { + this.activeLineController.dispose() + this.activeLineController = undefined + } + await this.closeAllDiffViews() this.editType = undefined this.isEditing = false @@ -606,8 +646,6 @@ export class DiffViewProvider { this.createdDirs = [] this.documentWasOpen = false this.activeDiffEditor = undefined - this.fadedOverlayController = undefined - this.activeLineController = undefined this.streamedLines = [] this.preDiagnostics = [] } diff --git a/src/integrations/editor/__tests__/DecorationController.spec.ts b/src/integrations/editor/__tests__/DecorationController.spec.ts new file mode 100644 index 0000000000..b59ce0e3b7 --- /dev/null +++ b/src/integrations/editor/__tests__/DecorationController.spec.ts @@ -0,0 +1,234 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import * as vscode from "vscode" +import { DecorationController } from "../DecorationController" + +// Mock vscode module +vi.mock("vscode", () => ({ + window: { + createTextEditorDecorationType: vi.fn().mockReturnValue({ + dispose: vi.fn(), + }), + }, + Range: vi.fn().mockImplementation((startLine, startChar, endLine, endChar) => ({ + start: { line: startLine, character: startChar }, + end: { line: endLine, character: endChar }, + with: vi.fn().mockReturnThis(), + })), + Position: vi.fn().mockImplementation((line, char) => ({ + line, + character: char, + translate: vi.fn().mockImplementation((lines) => ({ + line: line + lines, + character: char, + })), + })), +})) + +describe("DecorationController", () => { + let mockEditor: any + let mockDecorationType: any + let controller: DecorationController + + beforeEach(() => { + vi.clearAllMocks() + + // Create mock decoration type + mockDecorationType = { + dispose: vi.fn(), + } + vi.mocked(vscode.window.createTextEditorDecorationType).mockReturnValue(mockDecorationType) + + // Create mock editor + mockEditor = { + document: { + lineCount: 100, + getText: vi.fn().mockReturnValue("mock content"), + }, + setDecorations: vi.fn(), + } + + // Create controller + controller = new DecorationController("fadedOverlay", mockEditor) + }) + + describe("addLines", () => { + it("should add decorations when editor is valid", () => { + controller.addLines(0, 10) + + expect(mockEditor.setDecorations).toHaveBeenCalledWith( + expect.any(Object), + expect.arrayContaining([expect.any(Object)]), + ) + }) + + it("should not throw when editor is disposed", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Should not throw + expect(() => controller.addLines(0, 10)).not.toThrow() + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + + it("should not add decorations after dispose is called", () => { + controller.dispose() + controller.addLines(0, 10) + + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + + it("should handle setDecorations throwing an error", () => { + mockEditor.setDecorations.mockImplementation(() => { + throw new Error("Editor disposed during operation") + }) + + // Should not throw + expect(() => controller.addLines(0, 10)).not.toThrow() + }) + }) + + describe("clear", () => { + it("should clear decorations when editor is valid", () => { + controller.clear() + + expect(mockEditor.setDecorations).toHaveBeenCalledWith(expect.any(Object), []) + }) + + it("should not throw when editor is disposed", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Should not throw + expect(() => controller.clear()).not.toThrow() + }) + + it("should not clear decorations after dispose is called", () => { + controller.dispose() + controller.clear() + + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + }) + + describe("updateOverlayAfterLine", () => { + it("should update overlay when editor is valid", () => { + controller.updateOverlayAfterLine(50, 100) + + expect(mockEditor.setDecorations).toHaveBeenCalledWith( + expect.any(Object), + expect.arrayContaining([expect.any(Object)]), + ) + }) + + it("should not throw when editor is disposed", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Should not throw + expect(() => controller.updateOverlayAfterLine(50, 100)).not.toThrow() + }) + + it("should not update overlay after dispose is called", () => { + controller.dispose() + controller.updateOverlayAfterLine(50, 100) + + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + }) + + describe("setActiveLine", () => { + it("should set active line when editor is valid", () => { + controller.setActiveLine(25) + + expect(mockEditor.setDecorations).toHaveBeenCalledWith( + expect.any(Object), + expect.arrayContaining([expect.any(Object)]), + ) + }) + + it("should not throw when editor is disposed", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Should not throw + expect(() => controller.setActiveLine(25)).not.toThrow() + }) + + it("should not set active line after dispose is called", () => { + controller.dispose() + controller.setActiveLine(25) + + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + }) + + describe("dispose", () => { + it("should mark controller as disposed and clear decorations", () => { + controller.dispose() + + // Try to use controller after dispose - should not throw + expect(() => controller.addLines(0, 10)).not.toThrow() + expect(() => controller.clear()).not.toThrow() + expect(() => controller.updateOverlayAfterLine(50, 100)).not.toThrow() + expect(() => controller.setActiveLine(25)).not.toThrow() + + // No operations should have been performed + expect(mockEditor.setDecorations).not.toHaveBeenCalled() + }) + + it("should handle clear during dispose even if editor is disposed", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Should not throw + expect(() => controller.dispose()).not.toThrow() + }) + }) + + describe("isEditorValid", () => { + it("should detect valid editor", () => { + // Access private method through any type + const isValid = (controller as any).isEditorValid() + expect(isValid).toBe(true) + }) + + it("should detect disposed editor", () => { + // Simulate disposed editor + mockEditor.document = undefined + Object.defineProperty(mockEditor, "document", { + get: () => { + throw new Error("Editor is disposed") + }, + }) + + // Access private method through any type + const isValid = (controller as any).isEditorValid() + expect(isValid).toBe(false) + }) + }) +})