diff --git a/src/core/checkpoints/__tests__/checkpoint.test.ts b/src/core/checkpoints/__tests__/checkpoint.test.ts new file mode 100644 index 0000000000..c45f951ac9 --- /dev/null +++ b/src/core/checkpoints/__tests__/checkpoint.test.ts @@ -0,0 +1,449 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" +import { Task } from "../../task/Task" +import { ClineProvider } from "../../webview/ClineProvider" +import { checkpointSave, checkpointRestore, checkpointDiff, getCheckpointService } from "../index" +import * as vscode from "vscode" + +// Mock vscode +vi.mock("vscode", () => ({ + window: { + showErrorMessage: vi.fn(), + createTextEditorDecorationType: vi.fn(() => ({})), + showInformationMessage: vi.fn(), + }, + Uri: { + file: vi.fn((path: string) => ({ fsPath: path })), + parse: vi.fn((uri: string) => ({ with: vi.fn(() => ({})) })), + }, + commands: { + executeCommand: vi.fn(), + }, +})) + +// Mock other dependencies +vi.mock("@roo-code/telemetry", () => ({ + TelemetryService: { + instance: { + captureCheckpointCreated: vi.fn(), + captureCheckpointRestored: vi.fn(), + captureCheckpointDiffed: vi.fn(), + }, + }, +})) + +vi.mock("../../../utils/path", () => ({ + getWorkspacePath: vi.fn(() => "/test/workspace"), +})) + +vi.mock("../../../services/checkpoints") + +describe("Checkpoint functionality", () => { + let mockProvider: any + let mockTask: any + let mockCheckpointService: any + + beforeEach(async () => { + // Create mock checkpoint service + mockCheckpointService = { + isInitialized: true, + saveCheckpoint: vi.fn().mockResolvedValue({ commit: "test-commit-hash" }), + restoreCheckpoint: vi.fn().mockResolvedValue(undefined), + getDiff: vi.fn().mockResolvedValue([]), + on: vi.fn(), + initShadowGit: vi.fn().mockResolvedValue(undefined), + } + + // Create mock provider + mockProvider = { + context: { + globalStorageUri: { fsPath: "/test/storage" }, + }, + log: vi.fn(), + postMessageToWebview: vi.fn(), + postStateToWebview: vi.fn(), + cancelTask: vi.fn(), + } + + // Create mock task + mockTask = { + taskId: "test-task-id", + enableCheckpoints: true, + checkpointService: mockCheckpointService, + checkpointServiceInitializing: false, + providerRef: { + deref: () => mockProvider, + }, + clineMessages: [], + apiConversationHistory: [], + pendingUserMessageCheckpoint: undefined, + say: vi.fn().mockResolvedValue(undefined), + overwriteClineMessages: vi.fn(), + overwriteApiConversationHistory: vi.fn(), + combineMessages: vi.fn().mockReturnValue([]), + } + + // Update the mock to return our mockCheckpointService + const checkpointsModule = await import("../../../services/checkpoints") + vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create).mockReturnValue(mockCheckpointService) + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + describe("checkpointSave", () => { + it("should wait for checkpoint service initialization before saving", async () => { + // Set up task with uninitialized service + mockCheckpointService.isInitialized = false + mockTask.checkpointService = mockCheckpointService + + // Simulate service initialization after a delay + setTimeout(() => { + mockCheckpointService.isInitialized = true + }, 100) + + // Call checkpointSave + const savePromise = checkpointSave(mockTask, true) + + // Wait for the save to complete + const result = await savePromise + + // saveCheckpoint should have been called + expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledWith( + expect.stringContaining("Task: test-task-id"), + { allowEmpty: true }, + ) + + // Result should contain the commit hash + expect(result).toEqual({ commit: "test-commit-hash" }) + + // Task should still have checkpoints enabled + expect(mockTask.enableCheckpoints).toBe(true) + }) + + it("should handle timeout when service doesn't initialize", async () => { + // Service never initializes + mockCheckpointService.isInitialized = false + + // Call checkpointSave with a task that has no checkpoint service + const taskWithNoService = { + ...mockTask, + checkpointService: undefined, + enableCheckpoints: false, + } + + const result = await checkpointSave(taskWithNoService, true) + + // Result should be undefined + expect(result).toBeUndefined() + + // saveCheckpoint should not have been called + expect(mockCheckpointService.saveCheckpoint).not.toHaveBeenCalled() + }) + + it("should preserve checkpoint data through message deletion flow", async () => { + // Initialize service + mockCheckpointService.isInitialized = true + mockTask.checkpointService = mockCheckpointService + + // Simulate saving checkpoint before user message + const checkpointResult = await checkpointSave(mockTask, true) + expect(checkpointResult).toEqual({ commit: "test-commit-hash" }) + + // Simulate setting pendingUserMessageCheckpoint + if (checkpointResult && "commit" in checkpointResult) { + mockTask.pendingUserMessageCheckpoint = { + hash: checkpointResult.commit, + timestamp: Date.now(), + type: "user_message", + } + } + + // Verify checkpoint data is preserved + expect(mockTask.pendingUserMessageCheckpoint).toBeDefined() + expect(mockTask.pendingUserMessageCheckpoint.hash).toBe("test-commit-hash") + + // Simulate message deletion and reinitialization + mockTask.clineMessages = [] + mockTask.checkpointService = mockCheckpointService // Keep service available + mockTask.checkpointServiceInitializing = false + + // Save checkpoint again after deletion + const newCheckpointResult = await checkpointSave(mockTask, true) + + // Should still work after reinitialization + expect(newCheckpointResult).toEqual({ commit: "test-commit-hash" }) + expect(mockTask.enableCheckpoints).toBe(true) + }) + + it("should prevent duplicate checkpoint operations for the same task", async () => { + // Start two checkpoint saves simultaneously + const promise1 = checkpointSave(mockTask) + const promise2 = checkpointSave(mockTask) + + // Wait for both promises + const [result1, result2] = await Promise.all([promise1, promise2]) + + // Both should return the same result + expect(result1).toEqual(result2) + + // saveCheckpoint should only be called once due to deduplication + expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledTimes(1) + }) + + it("should handle errors gracefully and disable checkpoints", async () => { + mockCheckpointService.saveCheckpoint.mockRejectedValue(new Error("Save failed")) + + const result = await checkpointSave(mockTask) + + expect(result).toBeUndefined() + expect(mockTask.enableCheckpoints).toBe(false) + }) + }) + + describe("checkpointRestore", () => { + beforeEach(() => { + mockTask.clineMessages = [ + { ts: 1, say: "user", text: "Message 1" }, + { ts: 2, say: "assistant", text: "Message 2" }, + { ts: 3, say: "user", text: "Message 3" }, + ] + mockTask.apiConversationHistory = [ + { ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] }, + { ts: 2, role: "assistant", content: [{ type: "text", text: "Message 2" }] }, + { ts: 3, role: "user", content: [{ type: "text", text: "Message 3" }] }, + ] + }) + + it("should restore checkpoint for delete operation", async () => { + await checkpointRestore(mockTask, { + ts: 2, + commitHash: "abc123", + mode: "restore", + operation: "delete", + }) + + expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123") + expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([ + { ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] }, + ]) + expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([{ ts: 1, say: "user", text: "Message 1" }]) + expect(mockProvider.cancelTask).toHaveBeenCalled() + }) + + it("should restore checkpoint for edit operation", async () => { + await checkpointRestore(mockTask, { + ts: 2, + commitHash: "abc123", + mode: "restore", + operation: "edit", + }) + + expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123") + expect(mockTask.overwriteApiConversationHistory).toHaveBeenCalledWith([ + { ts: 1, role: "user", content: [{ type: "text", text: "Message 1" }] }, + ]) + // For edit operation, should include the message being edited + expect(mockTask.overwriteClineMessages).toHaveBeenCalledWith([ + { ts: 1, say: "user", text: "Message 1" }, + { ts: 2, say: "assistant", text: "Message 2" }, + ]) + expect(mockProvider.cancelTask).toHaveBeenCalled() + }) + + it("should handle preview mode without modifying messages", async () => { + await checkpointRestore(mockTask, { + ts: 2, + commitHash: "abc123", + mode: "preview", + }) + + expect(mockCheckpointService.restoreCheckpoint).toHaveBeenCalledWith("abc123") + expect(mockTask.overwriteApiConversationHistory).not.toHaveBeenCalled() + expect(mockTask.overwriteClineMessages).not.toHaveBeenCalled() + expect(mockProvider.cancelTask).toHaveBeenCalled() + }) + + it("should handle missing message gracefully", async () => { + await checkpointRestore(mockTask, { + ts: 999, // Non-existent timestamp + commitHash: "abc123", + mode: "restore", + }) + + expect(mockCheckpointService.restoreCheckpoint).not.toHaveBeenCalled() + }) + + it("should disable checkpoints on error", async () => { + mockCheckpointService.restoreCheckpoint.mockRejectedValue(new Error("Restore failed")) + + await checkpointRestore(mockTask, { + ts: 2, + commitHash: "abc123", + mode: "restore", + }) + + expect(mockTask.enableCheckpoints).toBe(false) + expect(mockProvider.log).toHaveBeenCalledWith("[checkpointRestore] disabling checkpoints for this task") + }) + }) + + describe("checkpointDiff", () => { + beforeEach(() => { + mockTask.clineMessages = [ + { ts: 1, say: "user", text: "Message 1" }, + { ts: 2, say: "checkpoint_saved", text: "commit1" }, + { ts: 3, say: "user", text: "Message 2" }, + { ts: 4, say: "checkpoint_saved", text: "commit2" }, + ] + }) + + it("should show diff for full mode", async () => { + const mockChanges = [ + { + paths: { absolute: "/test/file.ts", relative: "file.ts" }, + content: { before: "old content", after: "new content" }, + }, + ] + mockCheckpointService.getDiff.mockResolvedValue(mockChanges) + + await checkpointDiff(mockTask, { + ts: 4, + commitHash: "commit2", + mode: "full", + }) + + expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({ + from: undefined, + to: "commit2", + }) + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "vscode.changes", + "Changes since task started", + expect.any(Array), + ) + }) + + it("should show diff for checkpoint mode with previous commit", async () => { + const mockChanges = [ + { + paths: { absolute: "/test/file.ts", relative: "file.ts" }, + content: { before: "old content", after: "new content" }, + }, + ] + mockCheckpointService.getDiff.mockResolvedValue(mockChanges) + + await checkpointDiff(mockTask, { + ts: 4, + previousCommitHash: "commit1", + commitHash: "commit2", + mode: "checkpoint", + }) + + expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({ + from: "commit1", + to: "commit2", + }) + expect(vscode.commands.executeCommand).toHaveBeenCalledWith( + "vscode.changes", + "Changes since previous checkpoint", + expect.any(Array), + ) + }) + + it("should find previous checkpoint automatically in checkpoint mode", async () => { + const mockChanges = [ + { + paths: { absolute: "/test/file.ts", relative: "file.ts" }, + content: { before: "old content", after: "new content" }, + }, + ] + mockCheckpointService.getDiff.mockResolvedValue(mockChanges) + + await checkpointDiff(mockTask, { + ts: 4, + commitHash: "commit2", + mode: "checkpoint", + }) + + expect(mockCheckpointService.getDiff).toHaveBeenCalledWith({ + from: "commit1", // Should find the previous checkpoint + to: "commit2", + }) + }) + + it("should show information message when no changes found", async () => { + mockCheckpointService.getDiff.mockResolvedValue([]) + + await checkpointDiff(mockTask, { + ts: 4, + commitHash: "commit2", + mode: "full", + }) + + expect(vscode.window.showInformationMessage).toHaveBeenCalledWith("No changes found.") + expect(vscode.commands.executeCommand).not.toHaveBeenCalled() + }) + + it("should disable checkpoints on error", async () => { + mockCheckpointService.getDiff.mockRejectedValue(new Error("Diff failed")) + + await checkpointDiff(mockTask, { + ts: 4, + commitHash: "commit2", + mode: "full", + }) + + expect(mockTask.enableCheckpoints).toBe(false) + expect(mockProvider.log).toHaveBeenCalledWith("[checkpointDiff] disabling checkpoints for this task") + }) + }) + + describe("getCheckpointService", () => { + it("should return existing service if available", () => { + const service = getCheckpointService(mockTask) + expect(service).toBe(mockCheckpointService) + }) + + it("should return undefined if checkpoints are disabled", () => { + mockTask.enableCheckpoints = false + const service = getCheckpointService(mockTask) + expect(service).toBeUndefined() + }) + + it("should return undefined if service is still initializing", () => { + mockTask.checkpointService = undefined + mockTask.checkpointServiceInitializing = true + const service = getCheckpointService(mockTask) + expect(service).toBeUndefined() + }) + + it("should create new service if none exists", async () => { + mockTask.checkpointService = undefined + mockTask.checkpointServiceInitializing = false + + const service = getCheckpointService(mockTask) + + const checkpointsModule = await import("../../../services/checkpoints") + expect(vi.mocked(checkpointsModule.RepoPerTaskCheckpointService.create)).toHaveBeenCalledWith({ + taskId: "test-task-id", + workspaceDir: "/test/workspace", + shadowDir: "/test/storage", + log: expect.any(Function), + }) + }) + + it("should disable checkpoints if workspace path is not found", async () => { + const pathModule = await import("../../../utils/path") + vi.mocked(pathModule.getWorkspacePath).mockReturnValue(null as any) + + mockTask.checkpointService = undefined + mockTask.checkpointServiceInitializing = false + + const service = getCheckpointService(mockTask) + + expect(service).toBeUndefined() + expect(mockTask.enableCheckpoints).toBe(false) + }) + }) +}) diff --git a/src/core/checkpoints/__tests__/checkpointAfterDelete.test.ts b/src/core/checkpoints/__tests__/checkpointAfterDelete.test.ts deleted file mode 100644 index 2e628d06e7..0000000000 --- a/src/core/checkpoints/__tests__/checkpointAfterDelete.test.ts +++ /dev/null @@ -1,168 +0,0 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from "vitest" -import { Task } from "../../task/Task" -import { ClineProvider } from "../../webview/ClineProvider" -import { checkpointSave } from "../index" -import * as vscode from "vscode" - -// Mock vscode -vi.mock("vscode", () => ({ - window: { - showErrorMessage: vi.fn(), - createTextEditorDecorationType: vi.fn(() => ({})), - showInformationMessage: vi.fn(), - }, - Uri: { - file: vi.fn((path: string) => ({ fsPath: path })), - parse: vi.fn((uri: string) => ({ with: vi.fn(() => ({})) })), - }, - commands: { - executeCommand: vi.fn(), - }, -})) - -// Mock other dependencies -vi.mock("@roo-code/telemetry", () => ({ - TelemetryService: { - instance: { - captureCheckpointCreated: vi.fn(), - }, - }, -})) - -vi.mock("../../../utils/path", () => ({ - getWorkspacePath: vi.fn(() => "/test/workspace"), -})) - -describe("Checkpoint after message deletion", () => { - let mockProvider: any - let mockTask: any - let mockCheckpointService: any - - beforeEach(() => { - // Create mock checkpoint service - mockCheckpointService = { - isInitialized: true, // Set to true by default for most tests - saveCheckpoint: vi.fn().mockResolvedValue({ commit: "test-commit-hash" }), - on: vi.fn(), - initShadowGit: vi.fn().mockResolvedValue(undefined), - } - - // Create mock provider - mockProvider = { - context: { - globalStorageUri: { fsPath: "/test/storage" }, - }, - log: vi.fn(), - postMessageToWebview: vi.fn(), - postStateToWebview: vi.fn(), - } - - // Create mock task - mockTask = { - taskId: "test-task-id", - enableCheckpoints: true, - checkpointService: mockCheckpointService, // Set the service directly for most tests - checkpointServiceInitializing: false, - providerRef: { - deref: () => mockProvider, - }, - clineMessages: [], - pendingUserMessageCheckpoint: undefined, - } - - // Mock the RepoPerTaskCheckpointService.create to return our mock - vi.mock("../../../services/checkpoints", () => ({ - RepoPerTaskCheckpointService: { - create: vi.fn(() => mockCheckpointService), - }, - })) - }) - - afterEach(() => { - vi.clearAllMocks() - }) - - it("should wait for checkpoint service initialization before saving", async () => { - // Set up task with uninitialized service - mockCheckpointService.isInitialized = false - mockTask.checkpointService = mockCheckpointService - - // Simulate service initialization after a delay - setTimeout(() => { - mockCheckpointService.isInitialized = true - }, 100) - - // Call checkpointSave - const savePromise = checkpointSave(mockTask, true) - - // Wait for the save to complete - const result = await savePromise - - // saveCheckpoint should have been called - expect(mockCheckpointService.saveCheckpoint).toHaveBeenCalledWith( - expect.stringContaining("Task: test-task-id"), - { allowEmpty: true }, - ) - - // Result should contain the commit hash - expect(result).toEqual({ commit: "test-commit-hash" }) - - // Task should still have checkpoints enabled - expect(mockTask.enableCheckpoints).toBe(true) - }) - - it("should handle timeout when service doesn't initialize", async () => { - // Service never initializes - mockCheckpointService.isInitialized = false - - // Call checkpointSave with a task that has no checkpoint service - const taskWithNoService = { - ...mockTask, - checkpointService: undefined, - enableCheckpoints: false, - } - - const result = await checkpointSave(taskWithNoService, true) - - // Result should be undefined - expect(result).toBeUndefined() - - // saveCheckpoint should not have been called - expect(mockCheckpointService.saveCheckpoint).not.toHaveBeenCalled() - }) - - it("should preserve checkpoint data through message deletion flow", async () => { - // Initialize service - mockCheckpointService.isInitialized = true - mockTask.checkpointService = mockCheckpointService - - // Simulate saving checkpoint before user message - const checkpointResult = await checkpointSave(mockTask, true) - expect(checkpointResult).toEqual({ commit: "test-commit-hash" }) - - // Simulate setting pendingUserMessageCheckpoint - if (checkpointResult && "commit" in checkpointResult) { - mockTask.pendingUserMessageCheckpoint = { - hash: checkpointResult.commit, - timestamp: Date.now(), - type: "user_message", - } - } - - // Verify checkpoint data is preserved - expect(mockTask.pendingUserMessageCheckpoint).toBeDefined() - expect(mockTask.pendingUserMessageCheckpoint.hash).toBe("test-commit-hash") - - // Simulate message deletion and reinitialization - mockTask.clineMessages = [] - mockTask.checkpointService = mockCheckpointService // Keep service available - mockTask.checkpointServiceInitializing = false - - // Save checkpoint again after deletion - const newCheckpointResult = await checkpointSave(mockTask, true) - - // Should still work after reinitialization - expect(newCheckpointResult).toEqual({ commit: "test-commit-hash" }) - expect(mockTask.enableCheckpoints).toBe(true) - }) -}) diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index 9d342d324c..d37913337a 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -1358,12 +1358,17 @@ describe("Cline", () => { }) it("should handle pending ask operations gracefully when task is aborted", async () => { - const [cline, task] = Task.create({ + const [cline, taskPromise] = Task.create({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", }) + // Handle the task promise to prevent unhandled rejection + taskPromise.catch(() => { + // Expected error when task is aborted + }) + // Start an ask operation but don't respond to it const askPromise = cline.ask("tool", "Test question") @@ -1381,12 +1386,17 @@ describe("Cline", () => { }) it("should not throw 'Current ask promise was ignored' error when task is aborted", async () => { - const [cline, task] = Task.create({ + const [cline, taskPromise] = Task.create({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", }) + // Handle the task promise to prevent unhandled rejection + taskPromise.catch(() => { + // Expected error when task is aborted + }) + // Start multiple ask operations const askPromise1 = cline.ask("tool", "Question 1") const askPromise2 = cline.ask("tool", "Question 2") @@ -1400,12 +1410,17 @@ describe("Cline", () => { }) it("should resolve pending ask with messageResponse when abortTask is called", async () => { - const [cline, task] = Task.create({ + const [cline, taskPromise] = Task.create({ provider: mockProvider, apiConfiguration: mockApiConfig, task: "test task", }) + // Handle the task promise to prevent unhandled rejection + taskPromise.catch(() => { + // Expected error when task is aborted + }) + // Spy on the ask response properties // Start an ask operation const askPromise = cline.ask("tool", "Test question") diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index dd9ee12bfc..15401ff8d6 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -46,6 +46,12 @@ vi.mock("axios", () => ({ vi.mock("../../../utils/safeWriteJson") +vi.mock("../../../utils/storage", () => ({ + getSettingsDirectoryPath: vi.fn().mockResolvedValue("/test/settings/path"), + getTaskDirectoryPath: vi.fn().mockResolvedValue("/test/task/path"), + getGlobalStoragePath: vi.fn().mockResolvedValue("/test/storage/path"), +})) + vi.mock("@modelcontextprotocol/sdk/types.js", () => ({ CallToolResultSchema: {}, ListResourcesResultSchema: {}, @@ -1171,8 +1177,8 @@ describe("ClineProvider", () => { const mockMessages = [ { ts: 1000, type: "say", say: "user_feedback" }, // User message 1 { ts: 2000, type: "say", say: "tool" }, // Tool message - { ts: 3000, type: "say", say: "text", value: 4000 }, // Message to delete - { ts: 4000, type: "say", say: "browser_action" }, // Response to delete + { ts: 3000, type: "say", say: "text" }, // Message before delete + { ts: 4000, type: "say", say: "browser_action" }, // Message to delete { ts: 5000, type: "say", say: "user_feedback" }, // Next user message { ts: 6000, type: "say", say: "user_feedback" }, // Final message ] as ClineMessage[] @@ -1208,22 +1214,28 @@ describe("ClineProvider", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 4000, + hasCheckpoint: false, }) // Simulate user confirming deletion through the dialog await messageHandler({ type: "deleteMessageConfirm", messageTs: 4000 }) // Verify only messages before the deleted message were kept - expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) + expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([ + mockMessages[0], + mockMessages[1], + mockMessages[2], + ]) // Verify only API messages before the deleted message were kept expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([ mockApiHistory[0], mockApiHistory[1], + mockApiHistory[2], ]) - // Verify initClineWithHistoryItem was called - expect((provider as any).initClineWithHistoryItem).toHaveBeenCalledWith({ id: "test-task-id" }) + // initClineWithHistoryItem is only called when restoring checkpoints or aborting tasks + expect((provider as any).initClineWithHistoryItem).not.toHaveBeenCalled() }) test("handles case when no current task exists", async () => { @@ -1253,8 +1265,8 @@ describe("ClineProvider", () => { const mockMessages = [ { ts: 1000, type: "say", say: "user_feedback" }, // User message 1 { ts: 2000, type: "say", say: "tool" }, // Tool message - { ts: 3000, type: "say", say: "text", value: 4000 }, // Message to edit - { ts: 4000, type: "say", say: "browser_action" }, // Response to edit + { ts: 3000, type: "say", say: "text" }, // Message before edit + { ts: 4000, type: "say", say: "browser_action" }, // Message to edit { ts: 5000, type: "say", say: "user_feedback" }, // Next user message { ts: 6000, type: "say", say: "user_feedback" }, // Final message ] as ClineMessage[] @@ -1301,6 +1313,8 @@ describe("ClineProvider", () => { type: "showEditMessageDialog", messageTs: 4000, text: "Edited message content", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming edit through the dialog @@ -1311,12 +1325,17 @@ describe("ClineProvider", () => { }) // Verify correct messages were kept (only messages before the edited one) - expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) + expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([ + mockMessages[0], + mockMessages[1], + mockMessages[2], + ]) // Verify correct API messages were kept (only messages before the edited one) expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([ mockApiHistory[0], mockApiHistory[1], + mockApiHistory[2], ]) // The new flow calls webviewMessageHandler recursively with askResponse @@ -2709,6 +2728,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 3000, text: "Edited message with preserved images", + hasCheckpoint: false, + images: undefined, }) // Simulate confirmation @@ -2718,9 +2739,9 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { text: "Edited message with preserved images", }) - // Verify messages were edited correctly - only the first message should remain - expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0]]) - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }]) + // Verify messages were edited correctly - messages up to the edited message should remain + expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }, { ts: 2000 }]) }) test("handles editing messages with file attachments", async () => { @@ -2761,6 +2782,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 3000, text: "Edited message with file attachment", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -2817,6 +2840,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 2000, text: "Edited message", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -2857,6 +2882,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 2000, text: "Edited message", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -2913,11 +2940,15 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 2000, text: "Edited message 1", + hasCheckpoint: false, + images: undefined, }) expect(mockPostMessage).toHaveBeenCalledWith({ type: "showEditMessageDialog", messageTs: 4000, text: "Edited message 2", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming both edits @@ -3103,6 +3134,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 5000, text: "Edited non-existent message", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -3143,6 +3176,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 5000, + hasCheckpoint: false, }) // Simulate user confirming the delete @@ -3194,6 +3228,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 2000, text: "Edited message", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -3233,6 +3269,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 2000, + hasCheckpoint: false, }) // Simulate user confirming the delete @@ -3286,6 +3323,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: 2000, text: largeEditedContent, + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit @@ -3328,18 +3367,23 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 3000, + hasCheckpoint: false, }) // Simulate user confirming the delete await messageHandler({ type: "deleteMessageConfirm", messageTs: 3000 }) - // Should handle large payloads without issues - expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0]]) - expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }]) + // Should handle large payloads without issues - keeps messages before the deleted one + expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) + expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }, { ts: 2000 }]) }) }) describe("Error Messaging and User Feedback", () => { + beforeEach(async () => { + await provider.resolveWebviewView(mockWebviewView) + }) + // Note: Error messaging test removed as the implementation may not have proper error handling in place test("provides user feedback for successful operations", async () => { @@ -3366,6 +3410,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 2000, + hasCheckpoint: false, }) // Simulate user confirming the delete @@ -3373,7 +3418,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { // Verify successful operation completed expect(mockCline.overwriteClineMessages).toHaveBeenCalled() - expect(provider.initClineWithHistoryItem).toHaveBeenCalled() + // initClineWithHistoryItem is only called when restoring checkpoints or aborting tasks expect(vscode.window.showErrorMessage).not.toHaveBeenCalled() }) @@ -3439,6 +3484,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { expect(mockPostMessage).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 1000, + hasCheckpoint: false, }) // Simulate user confirming the delete @@ -3489,6 +3535,8 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => { type: "showEditMessageDialog", messageTs: futureTimestamp + 1000, text: "Edited future message", + hasCheckpoint: false, + images: undefined, }) // Simulate user confirming the edit diff --git a/src/core/webview/__tests__/checkpointRestoreHandler.test.ts b/src/core/webview/__tests__/checkpointRestoreHandler.test.ts index 468e5188d5..b01c2d47a2 100644 --- a/src/core/webview/__tests__/checkpointRestoreHandler.test.ts +++ b/src/core/webview/__tests__/checkpointRestoreHandler.test.ts @@ -45,28 +45,6 @@ describe("checkpointRestoreHandler", () => { } }) - describe("hasValidCheckpoint", () => { - it("should return true for valid checkpoint", () => { - const message = { checkpoint: { hash: "abc123" } } - expect(hasValidCheckpoint(message)).toBe(true) - }) - - it("should return false for missing checkpoint", () => { - const message = { text: "No checkpoint" } - expect(hasValidCheckpoint(message)).toBe(false) - }) - - it("should return false for invalid checkpoint structure", () => { - expect(hasValidCheckpoint({ checkpoint: "invalid" })).toBe(false) - expect(hasValidCheckpoint({ checkpoint: {} })).toBe(false) - expect(hasValidCheckpoint({ checkpoint: { hash: 123 } })).toBe(false) - }) - - it("should return false for empty hash", () => { - expect(hasValidCheckpoint({ checkpoint: { hash: "" } })).toBe(false) - }) - }) - describe("handleCheckpointRestoreOperation", () => { describe("delete operation", () => { it("should handle delete operation correctly", async () => { diff --git a/src/core/webview/__tests__/webviewMessageHandler.checkpoint.test.ts b/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts similarity index 95% rename from src/core/webview/__tests__/webviewMessageHandler.checkpoint.test.ts rename to src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts index fe42abd11a..c2d988bfcb 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.checkpoint.test.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.checkpoint.spec.ts @@ -10,6 +10,9 @@ vi.mock("vscode", () => ({ window: { showErrorMessage: vi.fn(), }, + workspace: { + workspaceFolders: undefined, + }, })) describe("webviewMessageHandler - checkpoint operations", () => { @@ -53,6 +56,7 @@ describe("webviewMessageHandler - checkpoint operations", () => { historyItem: { id: "test-task-123", messages: mockCline.clineMessages }, })), initClineWithHistoryItem: vi.fn(), + setPendingEditOperation: vi.fn(), contextProxy: { globalStorageUri: { fsPath: "/test/storage" }, }, @@ -136,8 +140,8 @@ describe("webviewMessageHandler - checkpoint operations", () => { operation: "edit", }) - // Verify the pending edit operation was stored - expect(mockCline.pendingEditOperation).toEqual({ + // Verify the pending edit operation was stored on the provider + expect(mockProvider.setPendingEditOperation).toHaveBeenCalledWith("task-test-task-123", { messageTs: 3, editedContent: "Edited checkpoint message", images: undefined, diff --git a/src/core/webview/__tests__/webviewMessageHandler.spec.ts b/src/core/webview/__tests__/webviewMessageHandler.spec.ts index 776ee2647b..f2052f5533 100644 --- a/src/core/webview/__tests__/webviewMessageHandler.spec.ts +++ b/src/core/webview/__tests__/webviewMessageHandler.spec.ts @@ -502,7 +502,10 @@ describe("webviewMessageHandler - message dialog preferences", () => { describe("deleteMessage", () => { it("should always show dialog for delete confirmation", async () => { - vi.mocked(mockClineProvider.getCurrentCline).mockReturnValue({} as any) // Mock current cline exists + vi.mocked(mockClineProvider.getCurrentCline).mockReturnValue({ + clineMessages: [], + apiConversationHistory: [], + } as any) // Mock current cline with proper structure await webviewMessageHandler(mockClineProvider, { type: "deleteMessage", @@ -512,13 +515,17 @@ describe("webviewMessageHandler - message dialog preferences", () => { expect(mockClineProvider.postMessageToWebview).toHaveBeenCalledWith({ type: "showDeleteMessageDialog", messageTs: 123456789, + hasCheckpoint: false, }) }) }) describe("submitEditedMessage", () => { it("should always show dialog for edit confirmation", async () => { - vi.mocked(mockClineProvider.getCurrentCline).mockReturnValue({} as any) // Mock current cline exists + vi.mocked(mockClineProvider.getCurrentCline).mockReturnValue({ + clineMessages: [], + apiConversationHistory: [], + } as any) // Mock current cline with proper structure await webviewMessageHandler(mockClineProvider, { type: "submitEditedMessage", diff --git a/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx index b85e0bc99c..93d01ea9f4 100644 --- a/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ChatTextArea.spec.tsx @@ -868,19 +868,15 @@ describe("ChatTextArea", () => { render() - // Should show edit button (codicon-edit) - const editButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-edit") !== null - }, + // Should show save button in edit mode + const saveButton = screen.getByRole("button", { + name: /save/i, }) - expect(editButton).toBeInTheDocument() + expect(saveButton).toBeInTheDocument() - // Should not show send button (codicon-send) + // Should not show send button in edit mode const sendButton = screen.queryByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-send") !== null - }, + name: /send/i, }) expect(sendButton).not.toBeInTheDocument() }) @@ -895,21 +891,17 @@ describe("ChatTextArea", () => { render() - // Should show send button (codicon-send) + // Should show send button when not in edit mode const sendButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-send") !== null - }, + name: /send/i, }) expect(sendButton).toBeInTheDocument() - // Should not show edit button (codicon-edit) - const editButton = screen.queryByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-edit") !== null - }, + // Should not show save button when not in edit mode + const saveButton = screen.queryByRole("button", { + name: /save/i, }) - expect(editButton).not.toBeInTheDocument() + expect(saveButton).not.toBeInTheDocument() }) it("should show cancel button in edit mode", () => { @@ -943,7 +935,7 @@ describe("ChatTextArea", () => { expect(cancelButton).not.toBeInTheDocument() }) - it("should call onSend when edit button is clicked", () => { + it("should call onSend when save button is clicked", () => { const onSend = vi.fn() ;(useExtensionState as ReturnType).mockReturnValue({ filePaths: [], @@ -954,13 +946,11 @@ describe("ChatTextArea", () => { render() - const editButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-edit") !== null - }, + const saveButton = screen.getByRole("button", { + name: /save/i, }) - fireEvent.click(editButton) + fireEvent.click(saveButton) expect(onSend).toHaveBeenCalledTimes(1) }) @@ -981,7 +971,7 @@ describe("ChatTextArea", () => { expect(onCancel).toHaveBeenCalledTimes(1) }) - it("should disable edit button when sendingDisabled is true", () => { + it("should disable save button when sendingDisabled is true", () => { ;(useExtensionState as ReturnType).mockReturnValue({ filePaths: [], openedTabs: [], @@ -991,13 +981,11 @@ describe("ChatTextArea", () => { render() - const editButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-edit") !== null - }, + const saveButton = screen.getByRole("button", { + name: /save/i, }) - expect(editButton).toBeDisabled() + expect(saveButton).toBeDisabled() }) it("should disable cancel button when sendingDisabled is true", () => { @@ -1014,7 +1002,7 @@ describe("ChatTextArea", () => { expect(cancelButton).toBeDisabled() }) - it("should have correct tooltip for edit button", () => { + it("should have correct tooltip for save button", () => { ;(useExtensionState as ReturnType).mockReturnValue({ filePaths: [], openedTabs: [], @@ -1024,17 +1012,16 @@ describe("ChatTextArea", () => { render() - const editButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-edit") !== null - }, + // Look for the save button by its aria-label + const saveButton = screen.getByRole("button", { + name: /save/i, }) // Check that the button has the correct aria-label (which is used for tooltip) - expect(editButton).toHaveAttribute("aria-label", expect.stringMatching(/save/i)) + expect(saveButton).toHaveAttribute("aria-label", expect.stringMatching(/save/i)) }) - it("should position cancel button correctly relative to camera button", () => { + it("should position cancel button correctly relative to image button", () => { ;(useExtensionState as ReturnType).mockReturnValue({ filePaths: [], openedTabs: [], @@ -1045,22 +1032,21 @@ describe("ChatTextArea", () => { const { container } = render() const cancelButton = screen.getByRole("button", { name: /cancel/i }) - const cameraButton = screen.getByRole("button", { - name: (_, element) => { - return element.querySelector(".codicon-device-camera") !== null - }, + // Look for the image button by its aria-label + const imageButton = screen.getByRole("button", { + name: /add.*images/i, }) // Both buttons should be in the same container (bottom toolbar) const bottomToolbar = container.querySelector(".flex.items-center.gap-0\\.5.shrink-0") expect(bottomToolbar).toContainElement(cancelButton) - expect(bottomToolbar).toContainElement(cameraButton) + expect(bottomToolbar).toContainElement(imageButton) - // Cancel button should come before camera button in DOM order + // Cancel button should come before image button in DOM order const buttons = bottomToolbar?.querySelectorAll("button") const cancelIndex = Array.from(buttons || []).indexOf(cancelButton as HTMLButtonElement) - const cameraIndex = Array.from(buttons || []).indexOf(cameraButton as HTMLButtonElement) - expect(cancelIndex).toBeLessThan(cameraIndex) + const imageIndex = Array.from(buttons || []).indexOf(imageButton as HTMLButtonElement) + expect(cancelIndex).toBeLessThan(imageIndex) }) }) }) diff --git a/webview-ui/src/components/chat/__tests__/CheckpointRestoreDialog.spec.tsx b/webview-ui/src/components/chat/__tests__/CheckpointRestoreDialog.spec.tsx index d76b110452..d43754475b 100644 --- a/webview-ui/src/components/chat/__tests__/CheckpointRestoreDialog.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CheckpointRestoreDialog.spec.tsx @@ -13,18 +13,14 @@ vi.mock("@src/i18n/TranslationContext", () => ({ const translations: Record = { "common:confirmation.delete_message": "Delete Message", "common:confirmation.edit_message": "Edit Message", - "common:confirmation.delete_warning": + "common:confirmation.delete_question_with_checkpoint": "Deleting this message will delete all subsequent messages in the conversation. Do you want to proceed?", - "common:confirmation.edit_warning": + "common:confirmation.edit_question_with_checkpoint": "Editing this message will delete all subsequent messages in the conversation. Do you want to proceed?", - "common:confirmation.delete_warning_with_checkpoint": - "Deleting this message will delete all subsequent messages in the conversation. Do you want to proceed?", - "common:confirmation.edit_warning_with_checkpoint": - "Editing this message will delete all subsequent messages in the conversation. Do you want to proceed?", - "common:confirmation.restore_checkpoint": "Do you also wish to revert code to this checkpoint?", - "common:confirmation.proceed": "Proceed", + "common:confirmation.edit_only": "Edit Only", + "common:confirmation.delete_only": "Delete Only", + "common:confirmation.restore_to_checkpoint": "Restore to Checkpoint", "common:answers.cancel": "Cancel", - "common:confirmation.dont_show_again": "Don't show this again", } return translations[key] || key }, @@ -54,9 +50,9 @@ describe("CheckpointRestoreDialog", () => { "Editing this message will delete all subsequent messages in the conversation. Do you want to proceed?", ), ).toBeInTheDocument() - expect(screen.getByText("Proceed")).toBeInTheDocument() + expect(screen.getByText("Edit Only")).toBeInTheDocument() expect(screen.getByText("Cancel")).toBeInTheDocument() - expect(screen.queryByText("Do you also wish to revert code to this checkpoint?")).not.toBeInTheDocument() + expect(screen.queryByText("Restore to Checkpoint")).not.toBeInTheDocument() }) it("renders delete dialog without checkpoint", () => { @@ -68,9 +64,9 @@ describe("CheckpointRestoreDialog", () => { "Deleting this message will delete all subsequent messages in the conversation. Do you want to proceed?", ), ).toBeInTheDocument() - expect(screen.getByText("Proceed")).toBeInTheDocument() + expect(screen.getByText("Delete Only")).toBeInTheDocument() expect(screen.getByText("Cancel")).toBeInTheDocument() - expect(screen.queryByText("Do you also wish to revert code to this checkpoint?")).not.toBeInTheDocument() + expect(screen.queryByText("Restore to Checkpoint")).not.toBeInTheDocument() }) it("renders edit dialog with checkpoint option", () => { @@ -82,8 +78,9 @@ describe("CheckpointRestoreDialog", () => { "Editing this message will delete all subsequent messages in the conversation. Do you want to proceed?", ), ).toBeInTheDocument() - expect(screen.getByText("Do you also wish to revert code to this checkpoint?")).toBeInTheDocument() - expect(screen.getAllByRole("checkbox")).toHaveLength(2) // restore and dont show again + expect(screen.getByText("Edit Only")).toBeInTheDocument() + expect(screen.getByText("Restore to Checkpoint")).toBeInTheDocument() + expect(screen.getByText("Cancel")).toBeInTheDocument() }) it("renders delete dialog with checkpoint option", () => { @@ -95,8 +92,9 @@ describe("CheckpointRestoreDialog", () => { "Deleting this message will delete all subsequent messages in the conversation. Do you want to proceed?", ), ).toBeInTheDocument() - expect(screen.getByText("Do you also wish to revert code to this checkpoint?")).toBeInTheDocument() - expect(screen.getAllByRole("checkbox")).toHaveLength(2) // restore and dont show again + expect(screen.getByText("Delete Only")).toBeInTheDocument() + expect(screen.getByText("Restore to Checkpoint")).toBeInTheDocument() + expect(screen.getByText("Cancel")).toBeInTheDocument() }) }) @@ -109,73 +107,36 @@ describe("CheckpointRestoreDialog", () => { expect(onOpenChange).toHaveBeenCalledWith(false) }) - it("calls onConfirm with correct parameters when proceed is clicked without checkpoint", () => { + it("calls onConfirm with correct parameters when edit only is clicked", () => { const onConfirm = vi.fn() render() - fireEvent.click(screen.getByText("Proceed")) - expect(onConfirm).toHaveBeenCalledWith(false, false) // dontShowAgain, restoreCheckpoint + fireEvent.click(screen.getByText("Edit Only")) + expect(onConfirm).toHaveBeenCalledWith(false) // restoreCheckpoint }) - it("calls onConfirm with restoreCheckpoint=false when proceed is clicked with unchecked checkbox", () => { + it("calls onConfirm with restoreCheckpoint=false when edit only is clicked with checkpoint", () => { const onConfirm = vi.fn() render() - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - expect(restoreCheckbox).not.toBeChecked() - - fireEvent.click(screen.getByText("Proceed")) - expect(onConfirm).toHaveBeenCalledWith(false, false) // dontShowAgain, restoreCheckpoint + fireEvent.click(screen.getByText("Edit Only")) + expect(onConfirm).toHaveBeenCalledWith(false) // restoreCheckpoint }) - it("calls onConfirm with restoreCheckpoint=true when proceed is clicked with checked checkbox", () => { + it("calls onConfirm with restoreCheckpoint=true when restore to checkpoint is clicked", () => { const onConfirm = vi.fn() render() - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - fireEvent.click(restoreCheckbox) - expect(restoreCheckbox).toBeChecked() - - fireEvent.click(screen.getByText("Proceed")) - expect(onConfirm).toHaveBeenCalledWith(false, true) // dontShowAgain, restoreCheckpoint + fireEvent.click(screen.getByText("Restore to Checkpoint")) + expect(onConfirm).toHaveBeenCalledWith(true) // restoreCheckpoint }) - it("toggles restore checkpoint checkbox state when clicked", () => { - render() + it("calls onOpenChange when dialog is closed", () => { + const onOpenChange = vi.fn() + render() - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - expect(restoreCheckbox).not.toBeChecked() - - fireEvent.click(restoreCheckbox) - expect(restoreCheckbox).toBeChecked() - - fireEvent.click(restoreCheckbox) - expect(restoreCheckbox).not.toBeChecked() - }) - - it("toggles dont show again checkbox state when clicked", () => { - render() - - const dontShowCheckbox = screen.getByLabelText("Don't show this again") - expect(dontShowCheckbox).not.toBeChecked() - - fireEvent.click(dontShowCheckbox) - expect(dontShowCheckbox).toBeChecked() - - fireEvent.click(dontShowCheckbox) - expect(dontShowCheckbox).not.toBeChecked() - }) - - it("calls onConfirm with dontShowAgain=true when dont show again is checked", () => { - const onConfirm = vi.fn() - render() - - const dontShowCheckbox = screen.getByLabelText("Don't show this again") - fireEvent.click(dontShowCheckbox) - expect(dontShowCheckbox).toBeChecked() - - fireEvent.click(screen.getByText("Proceed")) - expect(onConfirm).toHaveBeenCalledWith(true, false) // dontShowAgain, restoreCheckpoint + fireEvent.click(screen.getByText("Edit Only")) + expect(onOpenChange).toHaveBeenCalledWith(false) }) }) @@ -187,29 +148,19 @@ describe("CheckpointRestoreDialog", () => { expect(screen.queryByText("Delete Message")).not.toBeInTheDocument() }) - it("resets checkbox states when dialog reopens", async () => { + it("maintains state when dialog stays open", async () => { const { rerender } = render() - // Check both checkboxes - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - const dontShowCheckbox = screen.getByLabelText("Don't show this again") + // Verify initial state + expect(screen.getByText("Edit Only")).toBeInTheDocument() + expect(screen.getByText("Restore to Checkpoint")).toBeInTheDocument() - fireEvent.click(restoreCheckbox) - fireEvent.click(dontShowCheckbox) - expect(restoreCheckbox).toBeChecked() - expect(dontShowCheckbox).toBeChecked() - - // Close dialog - rerender() - - // Reopen dialog - useEffect should reset state when open becomes true + // Re-render with same props rerender() - // Checkboxes should be unchecked after reopening due to useEffect - const newRestoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - const newDontShowCheckbox = screen.getByLabelText("Don't show this again") - expect(newRestoreCheckbox).not.toBeChecked() - expect(newDontShowCheckbox).not.toBeChecked() + // Should still have same buttons + expect(screen.getByText("Edit Only")).toBeInTheDocument() + expect(screen.getByText("Restore to Checkpoint")).toBeInTheDocument() }) }) @@ -218,20 +169,10 @@ describe("CheckpointRestoreDialog", () => { render() expect(screen.getByRole("alertdialog")).toBeInTheDocument() // AlertDialog uses alertdialog role - expect(screen.getAllByRole("checkbox")).toHaveLength(2) // restore and dont show again - expect(screen.getByRole("button", { name: "Proceed" })).toBeInTheDocument() + expect(screen.getByRole("button", { name: "Edit Only" })).toBeInTheDocument() + expect(screen.getByRole("button", { name: "Restore to Checkpoint" })).toBeInTheDocument() expect(screen.getByRole("button", { name: "Cancel" })).toBeInTheDocument() }) - - it("checkboxes are properly labeled", () => { - render() - - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - const dontShowCheckbox = screen.getByLabelText("Don't show this again") - - expect(restoreCheckbox).toBeInTheDocument() - expect(dontShowCheckbox).toBeInTheDocument() - }) }) describe("Edge Cases", () => { @@ -244,20 +185,27 @@ describe("CheckpointRestoreDialog", () => { expect(screen.getByText("Edit Message")).toBeInTheDocument() }) - it("handles rapid state changes", async () => { + it("handles rapid button clicks", async () => { const onConfirm = vi.fn() - render() + const onOpenChange = vi.fn() + render( + , + ) - const restoreCheckbox = screen.getByLabelText("Do you also wish to revert code to this checkpoint?") - const proceedButton = screen.getByText("Proceed") + const editOnlyButton = screen.getByText("Edit Only") - // Rapidly toggle checkbox and click proceed - fireEvent.click(restoreCheckbox) - fireEvent.click(restoreCheckbox) - fireEvent.click(restoreCheckbox) - fireEvent.click(proceedButton) + // Click button once + fireEvent.click(editOnlyButton) - expect(onConfirm).toHaveBeenCalledWith(false, true) // dontShowAgain, restoreCheckpoint + // Should be called once with correct parameters + expect(onConfirm).toHaveBeenCalledTimes(1) + expect(onConfirm).toHaveBeenCalledWith(false) // restoreCheckpoint + expect(onOpenChange).toHaveBeenCalledWith(false) // dialog should close }) })