mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-07 08:26:51 +00:00
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
This commit is contained in:
parent
37300ef0c5
commit
6b68b164cf
5 changed files with 406 additions and 40 deletions
|
|
@ -238,7 +238,16 @@ export const openClineInNewTab = async ({ context, outputChannel }: Omit<Registe
|
|||
}
|
||||
|
||||
const tabProvider = new ClineProvider(context, outputChannel, "editor", contextProxy, codeIndexManager, mdmService)
|
||||
const lastCol = Math.max(...vscode.window.visibleTextEditors.map((editor) => 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.
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
||||
// 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 = []
|
||||
}
|
||||
|
|
|
|||
234
src/integrations/editor/__tests__/DecorationController.spec.ts
Normal file
234
src/integrations/editor/__tests__/DecorationController.spec.ts
Normal file
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
})
|
||||
Loading…
Add table
Reference in a new issue