Skip to content

Commit 13b6032

Browse files
committed
feat(editor): async post-save diagnostics on chat-diff save path (L1, #1375)
1 parent 1a2ade2 commit 13b6032

2 files changed

Lines changed: 111 additions & 21 deletions

File tree

‎src/integrations/editor/DiffViewProvider.ts‎

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1180,9 +1180,37 @@ export class DiffViewProvider {
11801180
await new Promise((resolve) => setTimeout(resolve, 100))
11811181
}
11821182

1183-
let newProblemsMessage = ""
1184-
1183+
// L1 (A2): resolve without awaiting the LSP diagnostics settle. The
1184+
// diagnostics check becomes a fire-and-forget tail that emits any new
1185+
// problems via the existing "error" ClineSay type; the returned
1186+
// newProblemsMessage is therefore always undefined.
11851187
if (diagnosticsEnabled) {
1188+
void this.emitPostSaveDiagnostics(relPath, writeDelayMs).catch((error) => {
1189+
console.warn(`Post-save diagnostics failed: ${error}`)
1190+
})
1191+
}
1192+
1193+
// Store the results for formatFileWriteResponse
1194+
this.newProblemsMessage = undefined
1195+
this.userEdits = undefined
1196+
this.relPath = relPath
1197+
this.newContent = content
1198+
1199+
return {
1200+
newProblemsMessage: undefined,
1201+
userEdits: undefined,
1202+
finalContent: content,
1203+
}
1204+
}
1205+
1206+
// L1 (A2): fire-and-forget post-save diagnostics. After the write delay,
1207+
// collects new Error-severity problems and emits them via the existing
1208+
// "error" ClineSay type (only Error-severity diagnostics reach this branch;
1209+
// "error" carries no task-failure semantics in core). Abort-safe: say()
1210+
// rejects on task abort, so the whole body is guarded and the tail is
1211+
// launched with an explicit .catch.
1212+
private async emitPostSaveDiagnostics(relPath: string, writeDelayMs: number): Promise<void> {
1213+
try {
11861214
// Add configurable delay to allow linters time to process
11871215
const safeDelayMs = Math.max(0, writeDelayMs)
11881216

@@ -1208,20 +1236,13 @@ export class DiffViewProvider {
12081236
maxDiagnosticMessages,
12091237
)
12101238

1211-
newProblemsMessage =
1212-
newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : ""
1213-
}
1214-
1215-
// Store the results for formatFileWriteResponse
1216-
this.newProblemsMessage = newProblemsMessage
1217-
this.userEdits = undefined
1218-
this.relPath = relPath
1219-
this.newContent = content
1220-
1221-
return {
1222-
newProblemsMessage,
1223-
userEdits: undefined,
1224-
finalContent: content,
1239+
if (newProblems.length > 0) {
1240+
await task?.say("error", `New problems detected after saving file: ${relPath}\n\n${newProblems}`)
1241+
}
1242+
} catch (error) {
1243+
// Abort-safe: never let a post-save diagnostic emit become an
1244+
// unhandled rejection (say() rejects when the task is aborted).
1245+
console.warn(`Post-save diagnostics emit failed: ${error}`)
12251246
}
12261247
}
12271248
}

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

Lines changed: 74 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ vi.mock("path", () => ({
3636
basename: vi.fn((path) => path.split("/").pop()),
3737
dirname: vi.fn((path) => path.split("/").slice(0, -1).join("/") || "/"),
3838
join: (...args: string[]) => args.join("/"),
39+
// diagnosticsToProblemsString formats its output header via
40+
// path.relative(cwd, uri.fsPath).toPosix(); the object-with-toPosix shape
41+
// mirrors the repo's own diagnostics.spec.ts mock.
42+
relative: vi.fn((cwd: string, p: string) => ({ toPosix: () => p.replace(`${cwd}/`, "") })),
3943
}))
4044

4145
// Mock vscode
@@ -174,6 +178,8 @@ describe("DiffViewProvider", () => {
174178
}),
175179
}),
176180
},
181+
// L1: saveDirectly's fire-and-forget diagnostics tail emits via say().
182+
say: vi.fn().mockResolvedValue(true),
177183
}
178184

179185
diffViewProvider = new DiffViewProvider(mockCwd, mockTask)
@@ -811,12 +817,17 @@ describe("DiffViewProvider", () => {
811817
{ preview: false, preserveFocus: true },
812818
)
813819

814-
// Verify diagnostics were checked after delay
820+
// L1: saveDirectly resolves before the fire-and-forget diagnostics
821+
// tail runs; flush one macrotask tick so the mocked delay (and the
822+
// post-write getDiagnostics) have been reached before asserting.
823+
await new Promise((resolve) => setTimeout(resolve, 0))
824+
825+
// Verify the tail applied the configured write delay
815826
expect(mockDelay).toHaveBeenCalledWith(2000)
816827
expect(vscode.languages.getDiagnostics).toHaveBeenCalled()
817828

818-
// Verify result
819-
expect(result.newProblemsMessage).toBe("")
829+
// Verify result: L1 no longer returns a problems message
830+
expect(result.newProblemsMessage).toBeUndefined()
820831
expect(result.userEdits).toBeUndefined()
821832
expect(result.finalContent).toBe("new content")
822833
})
@@ -847,6 +858,10 @@ describe("DiffViewProvider", () => {
847858
expect(mockDelay).not.toHaveBeenCalled()
848859
// getDiagnostics is called once for pre-diagnostics, but not for post-diagnostics
849860
expect(vscode.languages.getDiagnostics).toHaveBeenCalledTimes(1)
861+
862+
// L1: no diagnostics tail is launched, so nothing is ever emitted
863+
await new Promise((resolve) => setTimeout(resolve, 0))
864+
expect(mockTask.say).not.toHaveBeenCalled()
850865
})
851866

852867
it("should handle negative delay values", async () => {
@@ -855,19 +870,73 @@ describe("DiffViewProvider", () => {
855870

856871
await diffViewProvider.saveDirectly("test.ts", "new content", true, true, -500)
857872

873+
// L1: the tail runs after resolve; flush one macrotask tick first.
874+
await new Promise((resolve) => setTimeout(resolve, 0))
875+
858876
// Verify delay was called with 0 (safe minimum)
859877
expect(mockDelay).toHaveBeenCalledWith(0)
860878
})
861879

862880
it("should store results for formatFileWriteResponse", async () => {
863881
await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 1000)
864882

865-
// Verify internal state was updated
866-
expect((diffViewProvider as any).newProblemsMessage).toBe("")
883+
// Verify internal state was updated (L1: the problems message is no
884+
// longer stored; it is emitted asynchronously via say("error"))
885+
expect((diffViewProvider as any).newProblemsMessage).toBeUndefined()
867886
expect((diffViewProvider as any).userEdits).toBeUndefined()
868887
expect((diffViewProvider as any).relPath).toBe("test.ts")
869888
expect((diffViewProvider as any).newContent).toBe("new content")
870889
})
890+
891+
it("resolves immediately and emits new problems via say('error') after the write delay", async () => {
892+
const mockDelay = vi.mocked(delay)
893+
mockDelay.mockClear()
894+
vi.mocked(vscode.languages.getDiagnostics).mockClear()
895+
896+
// Pre-write diagnostics are empty; the post-write snapshot (read by
897+
// the fire-and-forget tail) reports one new Error-severity problem.
898+
// vscode.workspace.fs.stat is an unimplemented vi.fn() mock, so
899+
// diagnosticsToProblemsString takes its "(unavailable)" fallback
900+
// branch and still formats the line.
901+
const newDiag: vscode.Diagnostic = {
902+
severity: vscode.DiagnosticSeverity.Error,
903+
range: new vscode.Range(0, 0, 0, 1),
904+
message: "boom",
905+
}
906+
const postDiagnostics: [vscode.Uri, vscode.Diagnostic[]][] = [[makeUri(`${mockCwd}/test.ts`), [newDiag]]]
907+
vi.mocked(vscode.languages.getDiagnostics).mockReturnValueOnce([]).mockReturnValue(postDiagnostics)
908+
909+
const result = await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 100)
910+
911+
// L1: saveDirectly resolves before the tail emits anything.
912+
expect(result.newProblemsMessage).toBeUndefined()
913+
expect(mockTask.say).not.toHaveBeenCalled()
914+
915+
// Flush the fire-and-forget tail (the mocked delay resolves immediately).
916+
await new Promise((resolve) => setTimeout(resolve, 0))
917+
918+
expect(mockDelay).toHaveBeenCalledWith(100)
919+
expect(mockTask.say).toHaveBeenCalledTimes(1)
920+
// The existing "error" ClineSay type is used, with the new-problems text.
921+
expect(mockTask.say).toHaveBeenCalledWith(
922+
"error",
923+
expect.stringContaining("New problems detected after saving file: test.ts"),
924+
)
925+
expect(mockTask.say.mock.calls[0]?.[1]).toContain("boom")
926+
})
927+
928+
it("never calls say when there are no new problems", async () => {
929+
vi.mocked(vscode.languages.getDiagnostics).mockClear()
930+
vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([])
931+
932+
await diffViewProvider.saveDirectly("test.ts", "new content", true, true, 50)
933+
934+
// Flush the fire-and-forget tail (pre/post snapshots are both empty,
935+
// so diagnosticsToProblemsString returns "" and nothing is emitted).
936+
await new Promise((resolve) => setTimeout(resolve, 0))
937+
938+
expect(mockTask.say).not.toHaveBeenCalled()
939+
})
871940
})
872941

873942
describe("saveChanges method with diagnostic settings", () => {

0 commit comments

Comments
 (0)