Skip to content

Commit d909a50

Browse files
committed
fix(webview): edited messages replace original and send immediately
- Truncate from preceding user_feedback message - Remove timestamp fallback logic - Update tests to match new behavior
1 parent 2625ede commit d909a50

File tree

5 files changed

+68
-120
lines changed

5 files changed

+68
-120
lines changed

src/core/webview/ClineProvider.ts

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -917,23 +917,9 @@ export class ClineProvider
917917
// Remove the target message and all subsequent messages
918918
await task.overwriteClineMessages(task.clineMessages.slice(0, messageIndex))
919919

920-
// Apply timestamp-based fallback if exact API index not found
921-
let effectiveApiIndex = apiConversationHistoryIndex
922-
if (apiConversationHistoryIndex === -1 && task.apiConversationHistory.length > 0) {
923-
// Find the first API message with timestamp >= messageTs
924-
// This ensures we remove any assistant responses that came after the edited user message
925-
const fallbackIndex = task.apiConversationHistory.findIndex((msg: any) => {
926-
return msg.ts && msg.ts >= pendingEdit.messageTs
927-
})
928-
929-
if (fallbackIndex !== -1) {
930-
effectiveApiIndex = fallbackIndex
931-
}
932-
}
933-
934-
if (effectiveApiIndex !== -1) {
920+
if (apiConversationHistoryIndex !== -1) {
935921
await task.overwriteApiConversationHistory(
936-
task.apiConversationHistory.slice(0, effectiveApiIndex),
922+
task.apiConversationHistory.slice(0, apiConversationHistoryIndex),
937923
)
938924
}
939925

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

Lines changed: 7 additions & 15 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
@@ -3046,9 +3038,9 @@ 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 }])
30523044
})
30533045

30543046
test("handles editing messages with file attachments", async () => {

src/test/webviewMessageHandler.delete.spec.ts renamed to src/core/webview/__tests__/webviewMessageHandler.delete.spec.ts

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
import { describe, it, expect, beforeEach, vi } from "vitest"
2-
import { webviewMessageHandler } from "../core/webview/webviewMessageHandler"
2+
import { webviewMessageHandler } from "../webviewMessageHandler"
33
import * as vscode from "vscode"
4-
import { ClineProvider } from "../core/webview/ClineProvider"
4+
import { ClineProvider } from "../ClineProvider"
55

66
// Mock the saveTaskMessages function
7-
vi.mock("../core/task-persistence", () => ({
7+
vi.mock("../../task-persistence", () => ({
88
saveTaskMessages: vi.fn(),
99
}))
1010

1111
// Mock the i18n module
12-
vi.mock("../i18n", () => ({
12+
vi.mock("../../../i18n", () => ({
1313
t: vi.fn((key: string) => key),
1414
changeLanguage: vi.fn(),
1515
}))
@@ -79,7 +79,7 @@ describe("webviewMessageHandler delete functionality", () => {
7979
})
8080

8181
describe("handleDeleteMessageConfirm", () => {
82-
it("should handle deletion when apiConversationHistoryIndex is -1 and use timestamp fallback", async () => {
82+
it("should handle deletion when apiConversationHistoryIndex is -1 (message not in API history)", async () => {
8383
// Setup test data with a user message and assistant response
8484
const userMessageTs = 1000
8585
const assistantMessageTs = 1001
@@ -110,12 +110,9 @@ describe("webviewMessageHandler delete functionality", () => {
110110
// Verify that clineMessages was truncated at the correct index
111111
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([])
112112

113-
// Verify that apiConversationHistory was truncated using the timestamp fallback
114-
// Since assistantMessageTs >= userMessageTs, it should be removed
115-
// Check if the function was called at all
116-
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalled()
117-
// Then check what it was called with
118-
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([])
113+
// When message is not found in API history (index is -1),
114+
// API history should not be modified
115+
expect(getCurrentTaskMock.overwriteApiConversationHistory).not.toHaveBeenCalled()
119116
})
120117

121118
it("should handle deletion when exact apiConversationHistoryIndex is found", async () => {
@@ -203,11 +200,11 @@ describe("webviewMessageHandler delete functionality", () => {
203200
messageTs: userMessageTs,
204201
})
205202

206-
// Verify that both arrays were properly truncated
203+
// Verify that clineMessages was truncated
207204
expect(getCurrentTaskMock.overwriteClineMessages).toHaveBeenCalledWith([])
208205

209-
// The timestamp fallback should remove everything >= userMessageTs
210-
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([])
206+
// API history should not be modified when message not found
207+
expect(getCurrentTaskMock.overwriteApiConversationHistory).not.toHaveBeenCalled()
211208
})
212209

213210
it("should preserve messages before the deleted one", async () => {
@@ -223,6 +220,7 @@ describe("webviewMessageHandler delete functionality", () => {
223220
getCurrentTaskMock.apiConversationHistory = [
224221
{ ts: 1000, role: "user", content: { type: "text", text: "First message" } },
225222
{ ts: 1500, role: "assistant", content: { type: "text", text: "First response" } },
223+
{ ts: messageTs, role: "user", content: { type: "text", text: "Delete this" } },
226224
{ ts: 2500, role: "assistant", content: { type: "text", text: "Response to delete" } },
227225
]
228226

@@ -237,7 +235,7 @@ describe("webviewMessageHandler delete functionality", () => {
237235
{ ts: 1500, say: "assistant", text: "First response" },
238236
])
239237

240-
// API history should be truncated using timestamp fallback
238+
// API history should be truncated at the exact index
241239
expect(getCurrentTaskMock.overwriteApiConversationHistory).toHaveBeenCalledWith([
242240
{ ts: 1000, role: "user", content: { type: "text", text: "First message" } },
243241
{ ts: 1500, role: "assistant", content: { type: "text", text: "First response" } },

src/core/webview/__tests__/webviewMessageHandler.edit.spec.ts

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
7171
} as unknown as ClineProvider
7272
})
7373

74-
it("should use timestamp-based fallback when apiConversationHistoryIndex is -1", async () => {
74+
it("should not modify API history when apiConversationHistoryIndex is -1", async () => {
7575
// Setup: User message followed by attempt_completion
7676
const userMessageTs = 1000
7777
const assistantMessageTs = 2000
@@ -134,15 +134,11 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
134134
[], // All messages before index 0 (empty array)
135135
)
136136

137-
// Verify that API history was truncated using timestamp fallback
138-
// The fallback should find the first message with ts >= userMessageTs (1000)
139-
// which is the assistant message at ts=2000
140-
expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith(
141-
[], // All messages before the assistant message (empty array)
142-
)
137+
// API history should not be modified when message not found
138+
expect(mockCurrentTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
143139
})
144140

145-
it("should preserve messages before the edited message when using timestamp fallback", async () => {
141+
it("should preserve messages before the edited message when message not in API history", async () => {
146142
const earlierMessageTs = 500
147143
const userMessageTs = 1000
148144
const assistantMessageTs = 2000
@@ -200,15 +196,8 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
200196
},
201197
])
202198

203-
// Verify API history was truncated to preserve earlier message
204-
// Fallback should find first message with ts >= 1000, which is ts=2000
205-
expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([
206-
{
207-
ts: earlierMessageTs,
208-
role: "user",
209-
content: [{ type: "text", text: "Earlier message" }],
210-
},
211-
])
199+
// API history should not be modified when message not found
200+
expect(mockCurrentTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
212201
})
213202

214203
it("should not use fallback when exact apiConversationHistoryIndex is found", async () => {
@@ -292,8 +281,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
292281
// UI messages truncated
293282
expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([])
294283

295-
// API history should not be truncated when fallback finds no matching messages
296-
// The effectiveApiIndex remains -1
284+
// API history should not be modified when no matching messages found
297285
expect(mockCurrentTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
298286
})
299287

@@ -321,7 +309,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
321309
// UI messages should be truncated
322310
expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([])
323311

324-
// API history truncation should not be called with empty history
312+
// API history should not be modified when message not found
325313
expect(mockCurrentTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
326314
})
327315

@@ -351,7 +339,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
351339
} as ClineMessage,
352340
]
353341

354-
// API history with attempt_completion tool use
342+
// API history with attempt_completion tool use (user message missing)
355343
mockCurrentTask.apiConversationHistory = [
356344
{
357345
ts: completionTs,
@@ -390,8 +378,7 @@ describe("webviewMessageHandler - Edit Message with Timestamp Fallback", () => {
390378
// UI messages truncated at edited message
391379
expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([])
392380

393-
// API history should be truncated to remove the completion and feedback
394-
// Fallback finds first message with ts >= 1000, which is completionTs (2000)
395-
expect(mockCurrentTask.overwriteApiConversationHistory).toHaveBeenCalledWith([])
381+
// API history should not be modified when message not found
382+
expect(mockCurrentTask.overwriteApiConversationHistory).not.toHaveBeenCalled()
396383
})
397384
})

src/core/webview/webviewMessageHandler.ts

Lines changed: 35 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -91,28 +91,13 @@ export const webviewMessageHandler = async (
9191
currentCline: any,
9292
messageIndex: number,
9393
apiConversationHistoryIndex: number,
94-
messageTs?: number,
9594
) => {
9695
// Delete this message and all that follow
9796
await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex))
9897

99-
// If apiConversationHistoryIndex is -1, use timestamp-based fallback
100-
let effectiveApiIndex = apiConversationHistoryIndex
101-
if (apiConversationHistoryIndex === -1 && messageTs && currentCline.apiConversationHistory.length > 0) {
102-
// Find the first API message with timestamp >= messageTs
103-
// This ensures we remove any assistant responses that came after the deleted user message
104-
const fallbackIndex = currentCline.apiConversationHistory.findIndex((msg: ApiMessage) => {
105-
return msg.ts && msg.ts >= messageTs
106-
})
107-
108-
if (fallbackIndex !== -1) {
109-
effectiveApiIndex = fallbackIndex
110-
}
111-
}
112-
113-
if (effectiveApiIndex !== -1) {
98+
if (apiConversationHistoryIndex !== -1) {
11499
await currentCline.overwriteApiConversationHistory(
115-
currentCline.apiConversationHistory.slice(0, effectiveApiIndex),
100+
currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex),
116101
)
117102
}
118103
}
@@ -204,12 +189,7 @@ export const webviewMessageHandler = async (
204189
}
205190

206191
// Delete this message and all subsequent messages
207-
await removeMessagesThisAndSubsequent(
208-
currentCline,
209-
messageIndex,
210-
apiConversationHistoryIndex,
211-
messageTs,
212-
)
192+
await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex)
213193

214194
// Restore checkpoint associations for preserved messages
215195
for (const [ts, checkpoint] of preservedCheckpoints) {
@@ -332,32 +312,41 @@ export const webviewMessageHandler = async (
332312
}
333313
}
334314

335-
// For non-checkpoint edits, preserve checkpoint associations for remaining messages
315+
// For non-checkpoint edits, remove the ORIGINAL user message being edited and all subsequent messages
316+
// Determine the correct starting index to delete from (prefer the last preceding user_feedback message)
317+
let deleteFromMessageIndex = messageIndex
318+
let deleteFromApiIndex = apiConversationHistoryIndex
319+
320+
// Find the nearest preceding user message to ensure we replace the original, not just the assistant reply
321+
for (let i = messageIndex; i >= 0; i--) {
322+
const m = currentCline.clineMessages[i]
323+
if (m?.say === "user_feedback") {
324+
deleteFromMessageIndex = i
325+
// Align API history truncation to the same user message timestamp if present
326+
const userTs = m.ts
327+
if (typeof userTs === "number") {
328+
const apiIdx = currentCline.apiConversationHistory.findIndex(
329+
(am: ApiMessage) => am.ts === userTs,
330+
)
331+
if (apiIdx !== -1) {
332+
deleteFromApiIndex = apiIdx
333+
}
334+
}
335+
break
336+
}
337+
}
338+
336339
// Store checkpoints from messages that will be preserved
337340
const preservedCheckpoints = new Map<number, any>()
338-
for (let i = 0; i < messageIndex; i++) {
341+
for (let i = 0; i < deleteFromMessageIndex; i++) {
339342
const msg = currentCline.clineMessages[i]
340343
if (msg?.checkpoint && msg.ts) {
341344
preservedCheckpoints.set(msg.ts, msg.checkpoint)
342345
}
343346
}
344347

345-
// Edit this message and delete subsequent
346-
// If apiConversationHistoryIndex is -1, use timestamp-based fallback
347-
let effectiveApiIndex = apiConversationHistoryIndex
348-
if (apiConversationHistoryIndex === -1 && currentCline.apiConversationHistory.length > 0) {
349-
// Find the first API message with timestamp >= messageTs
350-
// This ensures we remove any assistant responses that came after the edited user message
351-
const fallbackIndex = currentCline.apiConversationHistory.findIndex((msg: ApiMessage) => {
352-
return msg.ts && msg.ts >= messageTs
353-
})
354-
355-
if (fallbackIndex !== -1) {
356-
effectiveApiIndex = fallbackIndex
357-
}
358-
}
359-
360-
await removeMessagesThisAndSubsequent(currentCline, messageIndex, effectiveApiIndex)
348+
// Delete the original (user) message and all subsequent messages
349+
await removeMessagesThisAndSubsequent(currentCline, deleteFromMessageIndex, deleteFromApiIndex)
361350

362351
// Restore checkpoint associations for preserved messages
363352
for (const [ts, checkpoint] of preservedCheckpoints) {
@@ -374,16 +363,12 @@ export const webviewMessageHandler = async (
374363
globalStoragePath: provider.contextProxy.globalStorageUri.fsPath,
375364
})
376365

377-
// Process the edited message as a regular user message
378-
webviewMessageHandler(provider, {
379-
type: "askResponse",
380-
askResponse: "messageResponse",
381-
text: editedContent,
382-
images,
383-
})
366+
// Update the UI to reflect the deletion
367+
await provider.postStateToWebview()
384368

385-
// Don't initialize with history item for edit operations
386-
// The webviewMessageHandler will handle the conversation state
369+
// Immediately send the edited message as a new user message
370+
// This restarts the conversation from the edited point
371+
await currentCline.handleWebviewAskResponse("messageResponse", editedContent, images)
387372
} catch (error) {
388373
console.error("Error in edit message:", error)
389374
vscode.window.showErrorMessage(

0 commit comments

Comments
 (0)