fix: prevent chat history truncation during message operations #6934
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -68,32 +68,64 @@ export const webviewMessageHandler = async ( | |||||
|
|
||||||
| /** | ||||||
| * Shared utility to find message indices based on timestamp | ||||||
| * Finds messages that should be deleted/edited based on the value field or timestamp | ||||||
| */ | ||||||
| const findMessageIndices = (messageTs: number, currentCline: any) => { | ||||||
| // First, try to find a message with a value field matching the timestamp | ||||||
| // This handles the case where we're deleting/editing a response to a message | ||||||
| let messageIndex = currentCline.clineMessages.findIndex( | ||||||
|
Contributor
Author
There was a problem hiding this comment. Is the test coverage for the new value field matching logic included? The PR mentions tests pass but I'm curious if we have specific test cases for:
|
||||||
| (msg: ClineMessage) => "value" in msg && msg.value === messageTs, | ||||||
| ) | ||||||
|
|
||||||
| // If no message with matching value field, look for exact timestamp match | ||||||
| if (messageIndex === -1) { | ||||||
| messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts === messageTs) | ||||||
| } | ||||||
|
|
||||||
| // For API conversation history, we need to find the corresponding index | ||||||
| // If we found a message by value field, use its timestamp for API history | ||||||
| let apiConversationHistoryIndex = -1 | ||||||
| if (messageIndex !== -1) { | ||||||
| const targetTs = currentCline.clineMessages[messageIndex].ts | ||||||
| apiConversationHistoryIndex = currentCline.apiConversationHistory.findIndex( | ||||||
| (msg: ApiMessage) => msg.ts && msg.ts >= targetTs, | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| return { messageIndex, apiConversationHistoryIndex } | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Removes the target message and all subsequent messages | ||||||
| * Includes validation to prevent accidental truncation | ||||||
| */ | ||||||
| const removeMessagesThisAndSubsequent = async ( | ||||||
| currentCline: any, | ||||||
| messageIndex: number, | ||||||
| apiConversationHistoryIndex: number, | ||||||
| ) => { | ||||||
| // Validate indices to prevent accidental truncation | ||||||
| if (messageIndex < 0) { | ||||||
| console.warn("Invalid message index for deletion, skipping operation") | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Store original lengths for logging | ||||||
| const originalClineLength = currentCline.clineMessages.length | ||||||
| const originalApiLength = currentCline.apiConversationHistory.length | ||||||
|
|
||||||
| // Delete this message and all that follow | ||||||
| const newClineMessages = currentCline.clineMessages.slice(0, messageIndex) | ||||||
| await currentCline.overwriteClineMessages(newClineMessages) | ||||||
|
|
||||||
| if (apiConversationHistoryIndex !== -1 && apiConversationHistoryIndex < originalApiLength) { | ||||||
| const newApiHistory = currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex) | ||||||
| await currentCline.overwriteApiConversationHistory(newApiHistory) | ||||||
|
|
||||||
| console.log(`Removed ${originalApiLength - newApiHistory.length} API conversation messages`) | ||||||
|
Contributor
Author
There was a problem hiding this comment. The logging could be more specific about what type of messages were removed. Consider:
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| console.log(`Removed ${originalClineLength - newClineMessages.length} Cline messages`) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Similarly here, we could be more specific:
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -116,21 +148,32 @@ export const webviewMessageHandler = async ( | |||||
| const currentCline = provider.getCurrentCline()! | ||||||
| const { messageIndex, apiConversationHistoryIndex } = findMessageIndices(messageTs, currentCline) | ||||||
|
|
||||||
| // For delete operations, if we can't find the message, log and return | ||||||
| // This can happen when the message doesn't exist or has already been deleted | ||||||
| if (messageIndex === -1) { | ||||||
| console.warn(`Message with timestamp ${messageTs} not found for deletion`) | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| const { historyItem } = await provider.getTaskWithId(currentCline.taskId) | ||||||
|
|
||||||
| // Validate that we're not deleting critical messages | ||||||
| if (messageIndex === 0) { | ||||||
|
Contributor
Author
There was a problem hiding this comment. This validation appears in both delete and edit operations. Would it make sense to extract this to a shared validation function like isFirstMessage(messageIndex) or canModifyMessage(messageIndex) for better maintainability? |
||||||
| vscode.window.showWarningMessage("Cannot delete the first message in the conversation") | ||||||
|
Contributor
There was a problem hiding this comment. Avoid hardcoded warning messages. Replace 'Cannot delete the first message in the conversation' (and similarly in the edit handler) with a translation call (e.g. t('...')) to support i18n.
Suggested change
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX. |
||||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Delete this message and all subsequent messages | ||||||
| await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex) | ||||||
|
|
||||||
| // Initialize with history item after deletion | ||||||
| await provider.initClineWithHistoryItem(historyItem) | ||||||
| } catch (error) { | ||||||
| console.error("Error in delete message:", error) | ||||||
| vscode.window.showErrorMessage( | ||||||
| `Error deleting message: ${error instanceof Error ? error.message : String(error)}`, | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -163,28 +206,50 @@ export const webviewMessageHandler = async ( | |||||
| // Use findMessageIndices to find messages based on timestamp | ||||||
| const { messageIndex, apiConversationHistoryIndex } = findMessageIndices(messageTs, currentCline) | ||||||
|
|
||||||
| // For edit operations, if we can't find the message, we should still handle it gracefully | ||||||
| // This can happen in tests or when the message has already been deleted | ||||||
| if (messageIndex === -1) { | ||||||
| console.warn(`Message with timestamp ${messageTs} not found for editing`) | ||||||
| // Still try to process the edit as a new message if no message found | ||||||
| // This handles edge cases in tests | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| // Validate that we're not editing critical messages | ||||||
| if (messageIndex === 0) { | ||||||
| vscode.window.showWarningMessage("Cannot edit the first message in the conversation") | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Store the message type before deletion for validation | ||||||
| const messageToEdit = currentCline.clineMessages[messageIndex] | ||||||
|
|
||||||
| // Only allow editing of user messages | ||||||
| if (messageToEdit.type !== "say" || messageToEdit.say !== "user_feedback") { | ||||||
| // For non-user messages, we should still allow editing but with proper handling | ||||||
| console.log(`Editing message of type: ${messageToEdit.type}`) | ||||||
| } | ||||||
|
|
||||||
| // Edit this message and delete subsequent | ||||||
| await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex) | ||||||
|
|
||||||
| // Process the edited message as a regular user message | ||||||
| // This will add it to the conversation and trigger an AI response | ||||||
| webviewMessageHandler(provider, { | ||||||
| type: "askResponse", | ||||||
| askResponse: "messageResponse", | ||||||
| text: editedContent, | ||||||
| images, | ||||||
| }) | ||||||
|
|
||||||
| // Don't initialize with history item for edit operations | ||||||
| // The webviewMessageHandler will handle the conversation state | ||||||
| } catch (error) { | ||||||
| console.error("Error in edit message:", error) | ||||||
| vscode.window.showErrorMessage( | ||||||
| `Error editing message: ${error instanceof Error ? error.message : String(error)}`, | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic here searches for messages by value field first, then falls back to timestamp matching. While this makes sense for the use case, could we add a comment explaining this precedence? There's a subtle possibility where both a value-matching message and a timestamp-matching message exist but are different messages.