feat(editor): async post-save diagnostics on chat-diff save path (L1, #1375)

Author: easonliang28

src/eslint-suppressions.json

@@ -1171,7 +1171,7 @@
 	},
 	"integrations/editor/__tests__/DiffViewProvider.spec.ts": {
 		"@typescript-eslint/no-explicit-any": {
-			"count": 310
+			"count": 306
 		}
 	},
 	"integrations/editor/__tests__/EditorUtils.spec.ts": {

src/integrations/editor/DiffViewProvider.ts

@@ -1152,8 +1152,11 @@ export class DiffViewProvider {
 	}> {
 		const absolutePath = path.resolve(this.cwd, relPath)
 
-		// Get diagnostics before editing the file
-		this.preDiagnostics = vscode.languages.getDiagnostics()
+		// Get diagnostics before editing the file. Capture the snapshot locally:
+		// overlapping saveDirectly calls (multi-file edits) must not let a later
+		// call overwrite this one's baseline before its diagnostics tail runs.
+		const preDiagnostics = vscode.languages.getDiagnostics()
+		this.preDiagnostics = preDiagnostics
 
 		// Write the content directly to the file
 		await createDirectoriesForFile(absolutePath)
@@ -1180,19 +1183,51 @@ export class DiffViewProvider {
 			await new Promise((resolve) => setTimeout(resolve, 100))
 		}
 
-		let newProblemsMessage = ""
-
+		// L1 (A2): resolve without awaiting the LSP diagnostics settle. The
+		// diagnostics check becomes a fire-and-forget tail that emits any new
+		// problems via the existing "error" ClineSay type; the returned
+		// newProblemsMessage is therefore always undefined.
 		if (diagnosticsEnabled) {
-			// Add configurable delay to allow linters time to process
-			const safeDelayMs = Math.max(0, writeDelayMs)
+			// The method's outer try/catch guarantees it never rejects, so the
+			// fire-and-forget call needs no .catch wrapper.
+			void this.emitPostSaveDiagnostics(relPath, writeDelayMs, preDiagnostics)
+		}
 
-			try {
-				await delay(safeDelayMs)
-			} catch (error) {
-				console.warn(`Failed to apply write delay: ${error}`)
-			}
+		// Store the results for formatFileWriteResponse
+		this.newProblemsMessage = undefined
+		this.userEdits = undefined
+		this.relPath = relPath
+		this.newContent = content
 
-			const postDiagnostics = vscode.languages.getDiagnostics()
+		return {
+			newProblemsMessage: undefined,
+			userEdits: undefined,
+			finalContent: content,
+		}
+	}
+
+	// L1 (A2): fire-and-forget post-save diagnostics. After the write delay,
+	// collects new Error-severity problems and emits them via the existing
+	// "error" ClineSay type (only Error-severity diagnostics reach this branch;
+	// "error" carries no task-failure semantics in core). Abort-safe: say()
+	// rejects when the task is aborted, so the whole body sits inside a
+	// try/catch that degrades to a console.warn — the tail can never reject.
+	private async emitPostSaveDiagnostics(
+		relPath: string,
+		writeDelayMs: number,
+		preDiagnostics: [vscode.Uri, vscode.Diagnostic[]][],
+	): Promise<void> {
+		try {
+			// Add configurable delay to allow linters time to process.
+			// delay() never rejects, so no catch is required here.
+			const safeDelayMs = Math.max(0, writeDelayMs)
+			await delay(safeDelayMs)
+
+			// Filter to the saved file: saveDirectly resolves before this tail
+			// completes, so in a multi-file write sequence (e.g. apply_patch)
+			// a later file's problems must not be attributed to this relPath.
+			const savedFilePath = path.resolve(this.cwd, relPath)
+			const postDiagnostics = vscode.languages.getDiagnostics().filter(([uri]) => uri.fsPath === savedFilePath)
 
 			// Get diagnostic settings from state
 			const task = this.taskRef.deref()
@@ -1201,27 +1236,20 @@ export class DiffViewProvider {
 			const maxDiagnosticMessages = state?.maxDiagnosticMessages ?? 50
 
 			const newProblems = await diagnosticsToProblemsString(
-				getNewDiagnostics(this.preDiagnostics, postDiagnostics),
+				getNewDiagnostics(preDiagnostics, postDiagnostics),
 				[vscode.DiagnosticSeverity.Error],
 				this.cwd,
 				includeDiagnosticMessages,
 				maxDiagnosticMessages,
 			)
 
-			newProblemsMessage =
-				newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : ""
-		}
-
-		// Store the results for formatFileWriteResponse
-		this.newProblemsMessage = newProblemsMessage
-		this.userEdits = undefined
-		this.relPath = relPath
-		this.newContent = content
-
-		return {
-			newProblemsMessage,
-			userEdits: undefined,
-			finalContent: content,
+			if (newProblems.length > 0) {
+				await task?.say("error", `New problems detected after saving file: ${relPath}\n\n${newProblems}`)
+			}
+		} catch (error) {
+			// Abort-safe: never let a post-save diagnostic emit become an
+			// unhandled rejection (say() rejects when the task is aborted).
+			console.warn(`Post-save diagnostics emit failed: ${error}`)
 		}
 	}
 }

src/integrations/editor/__tests__/DiffViewProvider.spec.ts

@@ -36,6 +36,10 @@ vi.mock("path", () => ({
 	basename: vi.fn((path) => path.split("/").pop()),
 	dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"),
 	join: (...args: string[]) => args.join("/"),
+	// diagnosticsToProblemsString formats its output header via
+	// path.relative(cwd, uri.fsPath).toPosix(); the object-with-toPosix shape
+	// mirrors the repo's own diagnostics.spec.ts mock.
+	relative: vi.fn((cwd: string, p: string) => ({ toPosix: () => p.replace(`${cwd}/`, "") })),
 }))
 
 // Mock vscode
@@ -174,6 +178,8 @@ describe("DiffViewProvider", () => {
 					}),
 				}),
 			},
+			// L1: saveDirectly's fire-and-forget diagnostics tail emits via say().
+			say: vi.fn().mockResolvedValue(true),
 		}
 
 		diffViewProvider = new DiffViewProvider(mockCwd, mockTask)
@@ -811,12 +817,17 @@ describe("DiffViewProvider", () => {
 				{ preview: false, preserveFocus: true },
 			)
 
-			// Verify diagnostics were checked after delay
+			// L1: saveDirectly resolves before the fire-and-forget diagnostics
+			// tail runs; flush one macrotask tick so the mocked delay (and the
+			// post-write getDiagnostics) have been reached before asserting.
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
+			// Verify the tail applied the configured write delay
 			expect(mockDelay).toHaveBeenCalledWith(2000)
 			expect(vscode.languages.getDiagnostics).toHaveBeenCalled()
 
-			// Verify result
-			expect(result.newProblemsMessage).toBe("")
+			// Verify result: L1 no longer returns a problems message
+			expect(result.newProblemsMessage).toBeUndefined()
 			expect(result.userEdits).toBeUndefined()
 			expect(result.finalContent).toBe("new content")
 		})
@@ -847,6 +858,10 @@ describe("DiffViewProvider", () => {
 			expect(mockDelay).not.toHaveBeenCalled()
 			// getDiagnostics is called once for pre-diagnostics, but not for post-diagnostics
 			expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1)
+
+			// L1: no diagnostics tail is launched, so nothing is ever emitted
+			await new Promise((resolve) => setTimeout(resolve, 0))
+			expect(mockTask.say).not.toHaveBeenCalled()
 		})
 
 		it("should handle negative delay values", async () => {
@@ -855,18 +870,129 @@ describe("DiffViewProvider", () => {
 
 			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, -500)
 
+			// L1: the tail runs after resolve; flush one macrotask tick first.
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
 			// Verify delay was called with 0 (safe minimum)
 			expect(mockDelay).toHaveBeenCalledWith(0)
 		})
 
 		it("should store results for formatFileWriteResponse", async () => {
 			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 1000)
 
-			// Verify internal state was updated
-			expect((diffViewProvider as any).newProblemsMessage).toBe("")
-			expect((diffViewProvider as any).userEdits).toBeUndefined()
-			expect((diffViewProvider as any).relPath).toBe("test.ts")
-			expect((diffViewProvider as any).newContent).toBe("new content")
+			// Verify internal state was updated (L1: the problems message is no
+			// longer stored; it is emitted asynchronously via say("error"))
+			expect(diffViewProvider["newProblemsMessage"]).toBeUndefined()
+			expect(diffViewProvider["userEdits"]).toBeUndefined()
+			expect(diffViewProvider["relPath"]).toBe("test.ts")
+			expect(diffViewProvider["newContent"]).toBe("new content")
+		})
+
+		it("resolves immediately and emits new problems via say('error') after the write delay", async () => {
+			const mockDelay = vi.mocked(delay)
+			mockDelay.mockClear()
+			vi.mocked(vscode.languages.getDiagnostics).mockClear()
+
+			// Pre-write diagnostics are empty; the post-write snapshot (read by
+			// the fire-and-forget tail) reports one new Error-severity problem.
+			// vscode.workspace.fs.stat is an unimplemented vi.fn() mock, so
+			// diagnosticsToProblemsString takes its "(unavailable)" fallback
+			// branch and still formats the line.
+			const newDiag: vscode.Diagnostic = {
+				severity: vscode.DiagnosticSeverity.Error,
+				range: new vscode.Range(0, 0, 0, 1),
+				message: "boom",
+			}
+			const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]]
+			vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics)
+
+			const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100)
+
+			// L1: saveDirectly resolves before the tail emits anything.
+			expect(result.newProblemsMessage).toBeUndefined()
+			expect(mockTask.say).not.toHaveBeenCalled()
+
+			// Flush the fire-and-forget tail (the mocked delay resolves immediately).
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
+			expect(mockDelay).toHaveBeenCalledWith(100)
+			expect(mockTask.say).toHaveBeenCalledTimes(1)
+			// The existing "error" ClineSay type is used, with the new-problems text.
+			expect(mockTask.say).toHaveBeenCalledWith(
+				"error",
+				expect.stringContaining("New problems detected after saving file: test.ts"),
+			)
+			expect(mockTask.say.mock.calls[0]?.[1]).toContain("boom")
+		})
+
+		it("attributes only the saved file's new problems to the saved file", async () => {
+			vi.mocked(vscode.languages.getDiagnostics).mockClear()
+
+			// Multi-file write sequence: a later file's new error must not be
+			// attributed to this tail's relPath by the workspace-wide snapshot.
+			const ownDiag: vscode.Diagnostic = {
+				severity: vscode.DiagnosticSeverity.Error,
+				range: new vscode.Range(0, 0, 0, 1),
+				message: "own-problem",
+			}
+			const otherDiag: vscode.Diagnostic = {
+				severity: vscode.DiagnosticSeverity.Error,
+				range: new vscode.Range(0, 0, 0, 1),
+				message: "other-file-problem",
+			}
+			const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [
+				[makeUri(`${mockCwd}/test.ts`), [ownDiag]],
+				[makeUri(`${mockCwd}/other.ts`), [otherDiag]],
+			]
+			vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics)
+
+			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100)
+
+			// Flush the fire-and-forget tail.
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
+			expect(mockTask.say).toHaveBeenCalledTimes(1)
+			expect(mockTask.say.mock.calls[0]?.[1]).toContain("own-problem")
+			expect(mockTask.say.mock.calls[0]?.[1]).not.toContain("other-file-problem")
+		})
+
+		it("never calls say when there are no new problems", async () => {
+			vi.mocked(vscode.languages.getDiagnostics).mockClear()
+			vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([])
+
+			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 50)
+
+			// Flush the fire-and-forget tail (pre/post snapshots are both empty,
+			// so diagnosticsToProblemsString returns "" and nothing is emitted).
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
+			expect(mockTask.say).not.toHaveBeenCalled()
+		})
+
+		it("logs a warning instead of an unhandled rejection when the post-save say() is aborted", async () => {
+			const consoleWarnSpy = vi.spyOn(console, "warn").mockImplementation(() => {})
+			vi.mocked(vscode.languages.getDiagnostics).mockClear()
+
+			// One new Error-severity problem so the tail reaches say().
+			const newDiag: vscode.Diagnostic = {
+				severity: vscode.DiagnosticSeverity.Error,
+				range: new vscode.Range(0, 0, 0, 1),
+				message: "boom",
+			}
+			const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]]
+			vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics)
+
+			// The task is aborted while the diagnostics tail is emitting: say() rejects.
+			mockTask.say.mockRejectedValueOnce(new Error("aborted"))
+
+			await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 0)
+			// Flush the fire-and-forget tail.
+			await new Promise((resolve) => setTimeout(resolve, 0))
+
+			// The method's outer catch swallows the rejection with a warning.
+			expect(consoleWarnSpy).toHaveBeenCalledWith(expect.stringContaining("Post-save diagnostics emit failed:"))
+
+			consoleWarnSpy.mockRestore()
 		})
 	})