From 701560823e25d25c5d01c82e8145fff2b2d817be Mon Sep 17 00:00:00 2001 From: Will Li Date: Mon, 14 Jul 2025 15:26:05 -0700 Subject: [PATCH] fix race cond and lint --- src/core/checkpoints/index.ts | 1 + .../checkpointRestoreHandler.spec.ts | 243 ++++++++++++++++++ .../checkpointRestoreHandler.test.ts | 148 ----------- .../webviewMessageHandler.checkpoint.spec.ts | 70 ++--- src/core/webview/checkpointRestoreHandler.ts | 14 + 5 files changed, 285 insertions(+), 191 deletions(-) create mode 100644 src/core/webview/__tests__/checkpointRestoreHandler.spec.ts delete mode 100644 src/core/webview/__tests__/checkpointRestoreHandler.test.ts diff --git a/src/core/checkpoints/index.ts b/src/core/checkpoints/index.ts index 6ece497bd7..97db01f208 100644 --- a/src/core/checkpoints/index.ts +++ b/src/core/checkpoints/index.ts @@ -179,6 +179,7 @@ export async function checkpointSave(cline: Task, force = false) { } catch (err) { console.error("[Task#checkpointSave] caught unexpected error, disabling checkpoints", err) cline.enableCheckpoints = false + return undefined } })() diff --git a/src/core/webview/__tests__/checkpointRestoreHandler.spec.ts b/src/core/webview/__tests__/checkpointRestoreHandler.spec.ts new file mode 100644 index 0000000000..a1454272c3 --- /dev/null +++ b/src/core/webview/__tests__/checkpointRestoreHandler.spec.ts @@ -0,0 +1,243 @@ +import { describe, it, expect, vi, beforeEach } from "vitest" +import { handleCheckpointRestoreOperation } from "../checkpointRestoreHandler" +import { saveTaskMessages } from "../../task-persistence" +import pWaitFor from "p-wait-for" +import * as vscode from "vscode" + +// Mock dependencies +vi.mock("../../task-persistence", () => ({ + saveTaskMessages: vi.fn(), +})) +vi.mock("p-wait-for") +vi.mock("vscode", () => ({ + window: { + showErrorMessage: vi.fn(), + }, +})) + +describe("checkpointRestoreHandler", () => { + let mockProvider: any + let mockCline: any + + beforeEach(() => { + vi.clearAllMocks() + + // Setup mock Cline instance + mockCline = { + taskId: "test-task-123", + abort: false, + abortTask: vi.fn(() => { + mockCline.abort = true + }), + checkpointRestore: vi.fn(), + clineMessages: [ + { ts: 1, type: "user", say: "user", text: "First message" }, + { ts: 2, type: "assistant", say: "assistant", text: "Response" }, + { + ts: 3, + type: "user", + say: "user", + text: "Checkpoint message", + checkpoint: { hash: "abc123" }, + }, + { ts: 4, type: "assistant", say: "assistant", text: "After checkpoint" }, + ], + } + + // Setup mock provider + mockProvider = { + getCurrentCline: vi.fn(() => mockCline), + postMessageToWebview: vi.fn(), + getTaskWithId: vi.fn(() => ({ + historyItem: { id: "test-task-123", messages: mockCline.clineMessages }, + })), + initClineWithHistoryItem: vi.fn(), + setPendingEditOperation: vi.fn(), + contextProxy: { + globalStorageUri: { fsPath: "/test/storage" }, + }, + } + + // Mock pWaitFor to resolve immediately + ;(pWaitFor as any).mockImplementation(async (condition: () => boolean) => { + // Simulate the condition being met + return Promise.resolve() + }) + }) + + describe("handleCheckpointRestoreOperation", () => { + it("should abort task before checkpoint restore for delete operations", async () => { + // Simulate a task that hasn't been aborted yet + mockCline.abort = false + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "delete", + }) + + // Verify abortTask was called before checkpointRestore + expect(mockCline.abortTask).toHaveBeenCalled() + expect(mockCline.checkpointRestore).toHaveBeenCalled() + + // Verify the order of operations + const abortOrder = mockCline.abortTask.mock.invocationCallOrder[0] + const restoreOrder = mockCline.checkpointRestore.mock.invocationCallOrder[0] + expect(abortOrder).toBeLessThan(restoreOrder) + }) + + it("should not abort task if already aborted", async () => { + // Simulate a task that's already aborted + mockCline.abort = true + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "delete", + }) + + // Verify abortTask was not called + expect(mockCline.abortTask).not.toHaveBeenCalled() + expect(mockCline.checkpointRestore).toHaveBeenCalled() + }) + + it("should handle edit operations with pending edit data", async () => { + const editData = { + editedContent: "Edited content", + images: ["image1.png"], + apiConversationHistoryIndex: 2, + } + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "edit", + editData, + }) + + // Verify abortTask was NOT called for edit operations + expect(mockCline.abortTask).not.toHaveBeenCalled() + + // Verify pending edit operation was set + expect(mockProvider.setPendingEditOperation).toHaveBeenCalledWith("task-test-task-123", { + messageTs: 3, + editedContent: "Edited content", + images: ["image1.png"], + messageIndex: 2, + apiConversationHistoryIndex: 2, + originalCheckpoint: { hash: "abc123" }, + }) + + // Verify checkpoint restore was called with edit operation + expect(mockCline.checkpointRestore).toHaveBeenCalledWith({ + ts: 3, + commitHash: "abc123", + mode: "restore", + operation: "edit", + }) + }) + + it("should save messages after delete operation", async () => { + // Mock the checkpoint restore to simulate message deletion + mockCline.checkpointRestore.mockImplementation(async () => { + mockCline.clineMessages = mockCline.clineMessages.slice(0, 2) + }) + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "delete", + }) + + // Verify saveTaskMessages was called + expect(saveTaskMessages).toHaveBeenCalledWith({ + messages: mockCline.clineMessages, + taskId: "test-task-123", + globalStoragePath: "/test/storage", + }) + + // Verify initClineWithHistoryItem was called + expect(mockProvider.initClineWithHistoryItem).toHaveBeenCalled() + }) + + it("should reinitialize task with correct history item after delete", async () => { + const expectedHistoryItem = { + id: "test-task-123", + messages: mockCline.clineMessages, + } + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "delete", + }) + + // Verify getTaskWithId was called + expect(mockProvider.getTaskWithId).toHaveBeenCalledWith("test-task-123") + + // Verify initClineWithHistoryItem was called with the correct history item + expect(mockProvider.initClineWithHistoryItem).toHaveBeenCalledWith(expectedHistoryItem) + }) + + it("should not save messages or reinitialize for edit operation", async () => { + const editData = { + editedContent: "Edited content", + images: [], + apiConversationHistoryIndex: 2, + } + + await handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "edit", + editData, + }) + + // Verify saveTaskMessages was NOT called for edit operation + expect(saveTaskMessages).not.toHaveBeenCalled() + + // Verify initClineWithHistoryItem was NOT called for edit operation + expect(mockProvider.initClineWithHistoryItem).not.toHaveBeenCalled() + }) + + it("should handle errors gracefully", async () => { + // Mock checkpoint restore to throw an error + mockCline.checkpointRestore.mockRejectedValue(new Error("Checkpoint restore failed")) + + // The function should throw and show an error message + await expect( + handleCheckpointRestoreOperation({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, + operation: "delete", + }), + ).rejects.toThrow("Checkpoint restore failed") + + // Verify error message was shown + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Error during checkpoint restore: Checkpoint restore failed", + ) + }) + }) +}) diff --git a/src/core/webview/__tests__/checkpointRestoreHandler.test.ts b/src/core/webview/__tests__/checkpointRestoreHandler.test.ts deleted file mode 100644 index b01c2d47a2..0000000000 --- a/src/core/webview/__tests__/checkpointRestoreHandler.test.ts +++ /dev/null @@ -1,148 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest" -import { handleCheckpointRestoreOperation } from "../checkpointRestoreHandler" -import { hasValidCheckpoint } from "../../checkpoints/utils" -import { saveTaskMessages } from "../../task-persistence" -import * as vscode from "vscode" - -vi.mock("../../task-persistence", () => ({ - saveTaskMessages: vi.fn(), -})) - -vi.mock("vscode", () => ({ - window: { - showErrorMessage: vi.fn(), - }, -})) - -describe("checkpointRestoreHandler", () => { - let mockProvider: any - let mockCline: any - - beforeEach(() => { - vi.clearAllMocks() - - mockProvider = { - contextProxy: { - globalStorageUri: { - fsPath: "/test/global/storage", - }, - }, - getTaskWithId: vi.fn().mockResolvedValue({ - historyItem: { id: "task123", messages: [] }, - }), - initClineWithHistoryItem: vi.fn(), - setPendingEditOperation: vi.fn(), - } - - mockCline = { - taskId: "task123", - clineMessages: [ - { ts: 1, text: "Message 1" }, - { ts: 2, text: "Message 2", checkpoint: { hash: "abc123" } }, - { ts: 3, text: "Message 3" }, - ], - checkpointRestore: vi.fn(), - } - }) - - describe("handleCheckpointRestoreOperation", () => { - describe("delete operation", () => { - it("should handle delete operation correctly", async () => { - await handleCheckpointRestoreOperation({ - provider: mockProvider, - currentCline: mockCline, - messageTs: 2, - messageIndex: 1, - checkpoint: { hash: "abc123" }, - operation: "delete", - }) - - // Should call checkpointRestore with correct params - expect(mockCline.checkpointRestore).toHaveBeenCalledWith({ - ts: 2, - commitHash: "abc123", - mode: "restore", - operation: "delete", - }) - - // Should save messages after restoration - expect(saveTaskMessages).toHaveBeenCalledWith({ - messages: mockCline.clineMessages, - taskId: "task123", - globalStoragePath: "/test/global/storage", - }) - - // Should reinitialize the task - expect(mockProvider.getTaskWithId).toHaveBeenCalledWith("task123") - expect(mockProvider.initClineWithHistoryItem).toHaveBeenCalledWith({ - id: "task123", - messages: [], - }) - }) - }) - - describe("edit operation", () => { - it("should handle edit operation correctly", async () => { - const editData = { - editedContent: "Edited content", - images: ["image1.png"], - apiConversationHistoryIndex: 1, - } - - await handleCheckpointRestoreOperation({ - provider: mockProvider, - currentCline: mockCline, - messageTs: 2, - messageIndex: 1, - checkpoint: { hash: "abc123" }, - operation: "edit", - editData, - }) - - // Should call setPendingEditOperation on provider - expect(mockProvider.setPendingEditOperation).toHaveBeenCalledWith("task-task123", { - messageTs: 2, - editedContent: "Edited content", - images: ["image1.png"], - messageIndex: 1, - apiConversationHistoryIndex: 1, - originalCheckpoint: { hash: "abc123" }, - }) - - // Should call checkpointRestore with correct params - expect(mockCline.checkpointRestore).toHaveBeenCalledWith({ - ts: 2, - commitHash: "abc123", - mode: "restore", - operation: "edit", - }) - - // Should NOT save messages or reinitialize for edit - expect(saveTaskMessages).not.toHaveBeenCalled() - expect(mockProvider.initClineWithHistoryItem).not.toHaveBeenCalled() - }) - }) - - describe("error handling", () => { - it("should handle errors and show error message", async () => { - const error = new Error("Checkpoint restore failed") - mockCline.checkpointRestore.mockRejectedValue(error) - - await expect( - handleCheckpointRestoreOperation({ - provider: mockProvider, - currentCline: mockCline, - messageTs: 2, - messageIndex: 1, - checkpoint: { hash: "abc123" }, - operation: "delete", - }), - ).rejects.toThrow("Checkpoint restore failed") - - expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( - "Error during checkpoint restore: Checkpoint restore failed", - ) - }) - }) - }) -}) diff --git a/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts index c2d988bfcb..5db10da16f 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts @@ -1,11 +1,11 @@ import { describe, it, expect, vi, beforeEach } from "vitest" import { webviewMessageHandler } from "../webviewMessageHandler" import { saveTaskMessages } from "../../task-persistence" -import { checkpointRestore } from "../../checkpoints" +import { handleCheckpointRestoreOperation } from "../checkpointRestoreHandler" // Mock dependencies vi.mock("../../task-persistence") -vi.mock("../../checkpoints") +vi.mock("../checkpointRestoreHandler") vi.mock("vscode", () => ({ window: { showErrorMessage: vi.fn(), @@ -33,7 +33,7 @@ describe("webviewMessageHandler - checkpoint operations", () => { type: "user", say: "user", text: "Checkpoint message", - checkpoint: { hash: "abc123", label: "Test checkpoint" }, + checkpoint: { hash: "abc123" }, }, { ts: 4, type: "assistant", say: "assistant", text: "After checkpoint" }, ], @@ -64,12 +64,9 @@ describe("webviewMessageHandler - checkpoint operations", () => { }) describe("delete operations with checkpoint restoration", () => { - it("should save messages to disk after checkpoint restoration", async () => { - // Simulate checkpoint restoration that removes messages - mockCline.checkpointRestore.mockImplementation(async () => { - // Simulate the effect of checkpoint restoration - mockCline.clineMessages = mockCline.clineMessages.slice(0, 2) - }) + it("should call handleCheckpointRestoreOperation for checkpoint deletes", async () => { + // Mock handleCheckpointRestoreOperation + ;(handleCheckpointRestoreOperation as any).mockResolvedValue(undefined) // Call the handler with delete confirmation await webviewMessageHandler(mockProvider, { @@ -78,25 +75,15 @@ describe("webviewMessageHandler - checkpoint operations", () => { restoreCheckpoint: true, }) - // Verify checkpoint restore was called with delete operation - expect(mockCline.checkpointRestore).toHaveBeenCalledWith({ - ts: 3, - commitHash: "abc123", - mode: "restore", + // Verify handleCheckpointRestoreOperation was called with correct parameters + expect(handleCheckpointRestoreOperation).toHaveBeenCalledWith({ + provider: mockProvider, + currentCline: mockCline, + messageTs: 3, + messageIndex: 2, + checkpoint: { hash: "abc123" }, operation: "delete", }) - - // Verify saveTaskMessages was called after checkpoint restoration - expect(saveTaskMessages).toHaveBeenCalledWith({ - messages: mockCline.clineMessages, - taskId: "test-task-123", - globalStoragePath: "/test/storage", - }) - - // Verify the save happened after the checkpoint restore - const checkpointRestoreOrder = mockCline.checkpointRestore.mock.invocationCallOrder[0] - const saveTaskMessagesOrder = (saveTaskMessages as any).mock.invocationCallOrder[0] - expect(saveTaskMessagesOrder).toBeGreaterThan(checkpointRestoreOrder) }) it("should save messages for non-checkpoint deletes", async () => { @@ -120,9 +107,9 @@ describe("webviewMessageHandler - checkpoint operations", () => { }) describe("edit operations with checkpoint restoration", () => { - it("should call checkpoint restore with edit operation", async () => { - // Mock the pending edit storage - mockCline.pendingEditOperation = null + it("should call handleCheckpointRestoreOperation for checkpoint edits", async () => { + // Mock handleCheckpointRestoreOperation + ;(handleCheckpointRestoreOperation as any).mockResolvedValue(undefined) // Call the handler with edit confirmation await webviewMessageHandler(mockProvider, { @@ -132,22 +119,19 @@ describe("webviewMessageHandler - checkpoint operations", () => { restoreCheckpoint: true, }) - // Verify checkpoint restore was called with edit operation - expect(mockCline.checkpointRestore).toHaveBeenCalledWith({ - ts: 3, - commitHash: "abc123", - mode: "restore", - operation: "edit", - }) - - // Verify the pending edit operation was stored on the provider - expect(mockProvider.setPendingEditOperation).toHaveBeenCalledWith("task-test-task-123", { + // Verify handleCheckpointRestoreOperation was called with correct parameters + expect(handleCheckpointRestoreOperation).toHaveBeenCalledWith({ + provider: mockProvider, + currentCline: mockCline, messageTs: 3, - editedContent: "Edited checkpoint message", - images: undefined, messageIndex: 2, - apiConversationHistoryIndex: 2, - originalCheckpoint: { hash: "abc123", label: "Test checkpoint" }, + checkpoint: { hash: "abc123" }, + operation: "edit", + editData: { + editedContent: "Edited checkpoint message", + images: undefined, + apiConversationHistoryIndex: 2, + }, }) }) }) diff --git a/src/core/webview/checkpointRestoreHandler.ts b/src/core/webview/checkpointRestoreHandler.ts index d25bf34ea6..ec9ab0c94f 100644 --- a/src/core/webview/checkpointRestoreHandler.ts +++ b/src/core/webview/checkpointRestoreHandler.ts @@ -28,6 +28,20 @@ export async function handleCheckpointRestoreOperation(config: CheckpointRestore const { provider, currentCline, messageTs, checkpoint, operation, editData } = config try { + // For delete operations, ensure the task is properly aborted to handle any pending ask operations + // This prevents "Current ask promise was ignored" errors + // For edit operations, we don't abort because the checkpoint restore will handle it + if (operation === "delete" && currentCline && !currentCline.abort) { + currentCline.abortTask() + // Wait a bit for the abort to complete + await pWaitFor(() => currentCline.abort === true, { + timeout: 1000, + interval: 50, + }).catch(() => { + // Continue even if timeout - the abort flag should be set + }) + } + // For edit operations, set up pending edit data before restoration if (operation === "edit" && editData) { const operationId = `task-${currentCline.taskId}`