Skip to content

Commit 88b5d66

Browse files
authored
fix: resolve chat message edit/delete duplication issues (#7793)
1 parent 5780659 commit 88b5d66

22 files changed

+917
-71
lines changed

src/core/webview/__tests__/ClineProvider.spec.ts

Lines changed: 20 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1332,19 +1332,11 @@ describe("ClineProvider", () => {
13321332
text: "Edited message content",
13331333
})
13341334

1335-
// Verify correct messages were kept (only messages before the edited one)
1336-
expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([
1337-
mockMessages[0],
1338-
mockMessages[1],
1339-
mockMessages[2],
1340-
])
1335+
// Verify correct messages were kept - delete from the preceding user message to truly replace it
1336+
expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([])
13411337

1342-
// Verify correct API messages were kept (only messages before the edited one)
1343-
expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([
1344-
mockApiHistory[0],
1345-
mockApiHistory[1],
1346-
mockApiHistory[2],
1347-
])
1338+
// Verify correct API messages were kept
1339+
expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([])
13481340

13491341
// The new flow calls webviewMessageHandler recursively with askResponse
13501342
// We need to verify the recursive call happened by checking if the handler was called again
@@ -3016,7 +3008,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
30163008
mockCline.apiConversationHistory = [{ ts: 1000 }, { ts: 2000 }, { ts: 3000 }] as any[]
30173009
mockCline.overwriteClineMessages = vi.fn()
30183010
mockCline.overwriteApiConversationHistory = vi.fn()
3019-
mockCline.handleWebviewAskResponse = vi.fn()
3011+
mockCline.submitUserMessage = vi.fn()
30203012

30213013
await provider.addClineToStack(mockCline)
30223014
;(provider as any).getTaskWithId = vi.fn().mockResolvedValue({
@@ -3046,9 +3038,11 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
30463038
text: "Edited message with preserved images",
30473039
})
30483040

3049-
// Verify messages were edited correctly - messages up to the edited message should remain
3050-
expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]])
3051-
expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }, { ts: 2000 }])
3041+
// Verify messages were edited correctly - the ORIGINAL user message and all subsequent messages are removed
3042+
expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0]])
3043+
expect(mockCline.overwriteApiConversationHistory).toHaveBeenCalledWith([{ ts: 1000 }])
3044+
// Verify submitUserMessage was called with the edited content
3045+
expect(mockCline.submitUserMessage).toHaveBeenCalledWith("Edited message with preserved images", undefined)
30523046
})
30533047

30543048
test("handles editing messages with file attachments", async () => {
@@ -3070,7 +3064,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
30703064
mockCline.apiConversationHistory = [{ ts: 1000 }, { ts: 2000 }, { ts: 3000 }] as any[]
30713065
mockCline.overwriteClineMessages = vi.fn()
30723066
mockCline.overwriteApiConversationHistory = vi.fn()
3073-
mockCline.handleWebviewAskResponse = vi.fn()
3067+
mockCline.submitUserMessage = vi.fn()
30743068

30753069
await provider.addClineToStack(mockCline)
30763070
;(provider as any).getTaskWithId = vi.fn().mockResolvedValue({
@@ -3101,11 +3095,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
31013095
})
31023096

31033097
expect(mockCline.overwriteClineMessages).toHaveBeenCalled()
3104-
expect(mockCline.handleWebviewAskResponse).toHaveBeenCalledWith(
3105-
"messageResponse",
3106-
"Edited message with file attachment",
3107-
undefined,
3108-
)
3098+
expect(mockCline.submitUserMessage).toHaveBeenCalledWith("Edited message with file attachment", undefined)
31093099
})
31103100
})
31113101

@@ -3197,7 +3187,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
31973187
await messageHandler({ type: "editMessageConfirm", messageTs: 2000, text: "Edited message" })
31983188

31993189
// The error should be caught and shown
3200-
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Error editing message: Connection lost")
3190+
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.message.error_editing_message")
32013191
})
32023192
})
32033193

@@ -3320,7 +3310,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
33203310
text: "Edited message",
33213311
})
33223312

3323-
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Error editing message: Unauthorized")
3313+
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.message.error_editing_message")
33243314
})
33253315

33263316
describe("Malformed Requests and Invalid Formats", () => {
@@ -3544,7 +3534,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
35443534

35453535
// Verify cleanup was attempted before failure
35463536
expect(cleanupSpy).toHaveBeenCalled()
3547-
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Error editing message: Operation failed")
3537+
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.message.error_editing_message")
35483538
})
35493539

35503540
test("validates proper cleanup during failed delete operations", async () => {
@@ -3584,9 +3574,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
35843574

35853575
// Verify cleanup was attempted before failure
35863576
expect(cleanupSpy).toHaveBeenCalled()
3587-
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith(
3588-
"Error deleting message: Delete operation failed",
3589-
)
3577+
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("errors.message.error_deleting_message")
35903578
})
35913579
})
35923580

@@ -3609,7 +3597,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
36093597
mockCline.apiConversationHistory = [{ ts: 1000 }, { ts: 2000 }] as any[]
36103598
mockCline.overwriteClineMessages = vi.fn()
36113599
mockCline.overwriteApiConversationHistory = vi.fn()
3612-
mockCline.handleWebviewAskResponse = vi.fn()
3600+
mockCline.submitUserMessage = vi.fn()
36133601

36143602
await provider.addClineToStack(mockCline)
36153603
;(provider as any).getTaskWithId = vi.fn().mockResolvedValue({
@@ -3638,11 +3626,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
36383626
await messageHandler({ type: "editMessageConfirm", messageTs: 2000, text: largeEditedContent })
36393627

36403628
expect(mockCline.overwriteClineMessages).toHaveBeenCalled()
3641-
expect(mockCline.handleWebviewAskResponse).toHaveBeenCalledWith(
3642-
"messageResponse",
3643-
largeEditedContent,
3644-
undefined,
3645-
)
3629+
expect(mockCline.submitUserMessage).toHaveBeenCalledWith(largeEditedContent, undefined)
36463630
})
36473631

36483632
test("handles deleting messages with large payloads", async () => {
@@ -3822,7 +3806,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
38223806
] as any[]
38233807
mockCline.overwriteClineMessages = vi.fn()
38243808
mockCline.overwriteApiConversationHistory = vi.fn()
3825-
mockCline.handleWebviewAskResponse = vi.fn()
3809+
mockCline.submitUserMessage = vi.fn()
38263810

38273811
await provider.addClineToStack(mockCline)
38283812
;(provider as any).getTaskWithId = vi.fn().mockResolvedValue({
@@ -3855,7 +3839,7 @@ describe("ClineProvider - Comprehensive Edit/Delete Edge Cases", () => {
38553839

38563840
// Should handle future timestamps correctly
38573841
expect(mockCline.overwriteClineMessages).toHaveBeenCalled()
3858-
expect(mockCline.handleWebviewAskResponse).toHaveBeenCalled()
3842+
expect(mockCline.submitUserMessage).toHaveBeenCalled()
38593843
})
38603844
})
38613845
})
Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
import { describe, it, expect, beforeEach, vi } from "vitest"
2+
import { webviewMessageHandler } from "../webviewMessageHandler"
3+
import * as vscode from "vscode"
4+
import { ClineProvider } from "../ClineProvider"
5+
6+
// Mock the saveTaskMessages function
7+
vi.mock("../../task-persistence", () => ({
8+
saveTaskMessages: vi.fn(),
9+
}))
10+
11+
// Mock the i18n module
12+
vi.mock("../../../i18n", () => ({
13+
t: vi.fn((key: string) => key),
14+
changeLanguage: vi.fn(),
15+
}))
16+
17+
vi.mock("vscode", () => ({
18+
window: {
19+
showErrorMessage: vi.fn(),
20+
showWarningMessage: vi.fn(),
21+
showInformationMessage: vi.fn(),
22+
},
23+
workspace: {
24+
workspaceFolders: undefined,
25+
getConfiguration: vi.fn(() => ({
26+
get: vi.fn(),
27+
update: vi.fn(),
28+
})),
29+
},
30+
ConfigurationTarget: {
31+
Global: 1,
32+
Workspace: 2,
33+
WorkspaceFolder: 3,
34+
},
35+
Uri: {
36+
parse: vi.fn((str) => ({ toString: () => str })),
37+
file: vi.fn((path) => ({ fsPath: path })),
38+
},
39+
env: {
40+
openExternal: vi.fn(),
41+
clipboard: {
42+
writeText: vi.fn(),
43+
},
44+
},
45+
commands: {
46+
executeCommand: vi.fn(),
47+
},
48+
}))
49+
50+
describe("webviewMessageHandler delete functionality", () => {
51+
let provider: any
52+
let getCurrentTaskMock: any
53+
54+
beforeEach(() => {
55+
// Reset all mocks
56+
vi.clearAllMocks()
57+
58+
// Create mock task
59+
getCurrentTaskMock = {
60+
clineMessages: [],
61+
apiConversationHistory: [],
62+
overwriteClineMessages: vi.fn(async () => {}),
63+
overwriteApiConversationHistory: vi.fn(async () => {}),
64+
taskId: "test-task-id",
65+
}
66+
67+
// Create mock provider
68+
provider = {
69+
getCurrentTask: vi.fn(() => getCurrentTaskMock),
70+
postMessageToWebview: vi.fn(),
71+
contextProxy: {
72+
getValue: vi.fn(),
73+
setValue: vi.fn(async () => {}),
74+
globalStorageUri: { fsPath: "/test/path" },
75+
},
76+
log: vi.fn(),
77+
cwd: "/test/cwd",
78+
}
79+
})
80+
81+
describe("handleDeleteMessageConfirm", () => {
82+
it("should handle deletion when apiConversationHistoryIndex is -1 (message not in API history)", async () => {
83+
// Setup test data with a user message and assistant response
84+
const userMessageTs = 1000
85+
const assistantMessageTs = 1001
86+
87+
getCurrentTaskMock.clineMessages = [
88+
{ ts: userMessageTs, say: "user", text: "Hello" },
89+
{ ts: assistantMessageTs, say: "assistant", text: "Hi there" },
90+
]
91+
92+
// API history has the assistant message but not the user message
93+
// This simulates the case where the user message wasn't in API history
94+
getCurrentTaskMock.apiConversationHistory = [
95+
{ ts: assistantMessageTs, role: "assistant", content: { type: "text", text: "Hi there" } },
96+
{
97+
ts: 1002,
98+
role: "assistant",
99+
content: { type: "text", text: "attempt_completion" },
100+
name: "attempt_completion",
101+
},
102+
]
103+
104+
// Call delete for the user message
105+
await webviewMessageHandler(provider, {
106+
type: "deleteMessageConfirm",
107+
messageTs: userMessageTs,
108+
})
109+
110+
// Verify that clineMessages was truncated at the correct index
111+
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([])
112+
113+
// When message is not found in API history (index is -1),
114+
// API history should be truncated from the first API message at/after the deleted timestamp (fallback)
115+
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([])
116+
})
117+
118+
it("should handle deletion when exact apiConversationHistoryIndex is found", async () => {
119+
// Setup test data where message exists in both arrays
120+
const messageTs = 1000
121+
122+
getCurrentTaskMock.clineMessages = [
123+
{ ts: 900, say: "user", text: "Previous message" },
124+
{ ts: messageTs, say: "user", text: "Delete this" },
125+
{ ts: 1100, say: "assistant", text: "Response" },
126+
]
127+
128+
getCurrentTaskMock.apiConversationHistory = [
129+
{ ts: 900, role: "user", content: { type: "text", text: "Previous message" } },
130+
{ ts: messageTs, role: "user", content: { type: "text", text: "Delete this" } },
131+
{ ts: 1100, role: "assistant", content: { type: "text", text: "Response" } },
132+
]
133+
134+
// Call delete
135+
await webviewMessageHandler(provider, {
136+
type: "deleteMessageConfirm",
137+
messageTs: messageTs,
138+
})
139+
140+
// Verify truncation at correct indices
141+
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([
142+
{ ts: 900, say: "user", text: "Previous message" },
143+
])
144+
145+
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([
146+
{ ts: 900, role: "user", content: { type: "text", text: "Previous message" } },
147+
])
148+
})
149+
150+
it("should handle deletion when message not found in clineMessages", async () => {
151+
getCurrentTaskMock.clineMessages = [{ ts: 1000, say: "user", text: "Some message" }]
152+
153+
getCurrentTaskMock.apiConversationHistory = []
154+
155+
// Call delete with non-existent timestamp
156+
await webviewMessageHandler(provider, {
157+
type: "deleteMessageConfirm",
158+
messageTs: 9999,
159+
})
160+
161+
// Verify error message was shown (expecting translation key since t() is mocked to return the key)
162+
expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("common:errors.message.message_not_found")
163+
164+
// Verify no truncation occurred
165+
expect(getCurrentTaskMock.overwriteClineMessages).not.toHaveBeenCalled()
166+
expect(getCurrentTaskMock.overwriteApiConversationHistory).not.toHaveBeenCalled()
167+
})
168+
169+
it("should handle deletion with attempt_completion in API history", async () => {
170+
// Setup test data with attempt_completion
171+
const userMessageTs = 1000
172+
const attemptCompletionTs = 1001
173+
174+
getCurrentTaskMock.clineMessages = [
175+
{ ts: userMessageTs, say: "user", text: "Fix the bug" },
176+
{ ts: attemptCompletionTs, say: "assistant", text: "I've fixed the bug" },
177+
]
178+
179+
// API history has attempt_completion but user message is missing
180+
getCurrentTaskMock.apiConversationHistory = [
181+
{
182+
ts: attemptCompletionTs,
183+
role: "assistant",
184+
content: {
185+
type: "text",
186+
text: "I've fixed the bug in the code",
187+
},
188+
name: "attempt_completion",
189+
},
190+
{
191+
ts: 1002,
192+
role: "user",
193+
content: { type: "text", text: "Looks good, but..." },
194+
},
195+
]
196+
197+
// Call delete for the user message
198+
await webviewMessageHandler(provider, {
199+
type: "deleteMessageConfirm",
200+
messageTs: userMessageTs,
201+
})
202+
203+
// Verify that clineMessages was truncated
204+
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([])
205+
206+
// API history should be truncated from first message at/after deleted timestamp (fallback)
207+
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([])
208+
})
209+
210+
it("should preserve messages before the deleted one", async () => {
211+
const messageTs = 2000
212+
213+
getCurrentTaskMock.clineMessages = [
214+
{ ts: 1000, say: "user", text: "First message" },
215+
{ ts: 1500, say: "assistant", text: "First response" },
216+
{ ts: messageTs, say: "user", text: "Delete this" },
217+
{ ts: 2500, say: "assistant", text: "Response to delete" },
218+
]
219+
220+
getCurrentTaskMock.apiConversationHistory = [
221+
{ ts: 1000, role: "user", content: { type: "text", text: "First message" } },
222+
{ ts: 1500, role: "assistant", content: { type: "text", text: "First response" } },
223+
{ ts: messageTs, role: "user", content: { type: "text", text: "Delete this" } },
224+
{ ts: 2500, role: "assistant", content: { type: "text", text: "Response to delete" } },
225+
]
226+
227+
await webviewMessageHandler(provider, {
228+
type: "deleteMessageConfirm",
229+
messageTs: messageTs,
230+
})
231+
232+
// Should preserve messages before the deleted one
233+
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([
234+
{ ts: 1000, say: "user", text: "First message" },
235+
{ ts: 1500, say: "assistant", text: "First response" },
236+
])
237+
238+
// API history should be truncated at the exact index
239+
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([
240+
{ ts: 1000, role: "user", content: { type: "text", text: "First message" } },
241+
{ ts: 1500, role: "assistant", content: { type: "text", text: "First response" } },
242+
])
243+
})
244+
})
245+
})

0 commit comments

Comments
 (0)