Fix chat history truncation during message operations #7142
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
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 |
|---|---|---|
|
|
@@ -67,32 +67,53 @@ export const webviewMessageHandler = async ( | |
| await provider.contextProxy.setValue(key, value) | ||
|
|
||
| /** | ||
| * Shared utility to find message indices based on exact timestamp matching | ||
|
Contributor
Author
There was a problem hiding this comment. Good addition of validation! Consider adding a comment explaining why we use exact timestamp matching instead of the previous buffer approach, perhaps referencing issue #6932 for future maintainers? |
||
| * This prevents accidental deletion of unrelated messages | ||
| */ | ||
| const findMessageIndices = (messageTs: number, currentCline: any) => { | ||
| // Use exact timestamp matching to prevent unintended message deletion | ||
| const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts === messageTs) | ||
|
Contributor
Author
There was a problem hiding this comment. While this exact timestamp matching fixes the immediate issue, consider what happens if multiple messages share the same timestamp. Would adding a secondary check using message content or index as a tiebreaker make this more robust? |
||
| const apiConversationHistoryIndex = currentCline.apiConversationHistory.findIndex( | ||
| (msg: ApiMessage) => msg.ts === messageTs, | ||
| ) | ||
| return { messageIndex, apiConversationHistoryIndex } | ||
| } | ||
|
|
||
| /** | ||
| * Removes the target message and all subsequent messages | ||
| * Includes validation to prevent accidental data loss | ||
| */ | ||
| const removeMessagesThisAndSubsequent = async ( | ||
| currentCline: any, | ||
| messageIndex: number, | ||
| apiConversationHistoryIndex: number, | ||
| ) => { | ||
| // Validate indices before deletion | ||
| if (messageIndex < 0 || messageIndex >= currentCline.clineMessages.length) { | ||
| console.error( | ||
| `[Chat History] Invalid message index: ${messageIndex}, total messages: ${currentCline.clineMessages.length}`, | ||
| ) | ||
| throw new Error("Invalid message index for deletion") | ||
| } | ||
|
|
||
| // Log deletion for debugging | ||
| const messagesToDelete = currentCline.clineMessages.length - messageIndex | ||
|
Contributor
Author
There was a problem hiding this comment. Nice defensive programming with the validation checks. For consistency, could we standardize all console logs in this file to use the prefix? Some logs have it, others don't. |
||
| console.log(`[Chat History] Deleting ${messagesToDelete} messages starting from index ${messageIndex}`) | ||
|
|
||
| // Delete this message and all that follow | ||
| await currentCline.overwriteClineMessages(currentCline.clineMessages.slice(0, messageIndex)) | ||
|
|
||
| if (apiConversationHistoryIndex !== -1) { | ||
| if ( | ||
| apiConversationHistoryIndex >= 0 && | ||
| apiConversationHistoryIndex < currentCline.apiConversationHistory.length | ||
| ) { | ||
| await currentCline.overwriteApiConversationHistory( | ||
| currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex), | ||
| ) | ||
| } else { | ||
| console.warn(`[Chat History] Invalid API conversation history index: ${apiConversationHistoryIndex}`) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -118,6 +139,16 @@ export const webviewMessageHandler = async ( | |
|
|
||
| if (messageIndex !== -1) { | ||
| try { | ||
| // Additional validation: ensure we're not deleting critical messages | ||
| const messageToDelete = currentCline.clineMessages[messageIndex] | ||
| if (!messageToDelete) { | ||
| console.error(`[Chat History] Message not found at index ${messageIndex}`) | ||
| throw new Error("Message not found for deletion") | ||
| } | ||
|
|
||
| // Log the message being deleted for debugging | ||
| console.log(`[Chat History] Deleting message with timestamp ${messageTs} at index ${messageIndex}`) | ||
|
|
||
| const { historyItem } = await provider.getTaskWithId(currentCline.taskId) | ||
|
|
||
| // Delete this message and all subsequent messages | ||
|
|
@@ -126,11 +157,13 @@ export const webviewMessageHandler = async ( | |
| // Initialize with history item after deletion | ||
| await provider.createTaskWithHistoryItem(historyItem) | ||
| } catch (error) { | ||
| console.error("[Chat History] Error in delete message:", error) | ||
| vscode.window.showErrorMessage( | ||
| `Error deleting message: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } else { | ||
| console.warn(`[Chat History] Message with timestamp ${messageTs} not found for deletion`) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -165,6 +198,16 @@ export const webviewMessageHandler = async ( | |
|
|
||
| if (messageIndex !== -1) { | ||
| try { | ||
| // Additional validation: ensure we're not editing critical messages | ||
| const messageToEdit = currentCline.clineMessages[messageIndex] | ||
| if (!messageToEdit) { | ||
| console.error(`[Chat History] Message not found at index ${messageIndex}`) | ||
| throw new Error("Message not found for editing") | ||
| } | ||
|
|
||
| // Log the message being edited for debugging | ||
| console.log(`[Chat History] Editing message with timestamp ${messageTs} at index ${messageIndex}`) | ||
|
|
||
| // Edit this message and delete subsequent | ||
| await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex) | ||
|
|
||
|
|
@@ -180,11 +223,13 @@ export const webviewMessageHandler = async ( | |
| // Don't initialize with history item for edit operations | ||
| // The webviewMessageHandler will handle the conversation state | ||
| } catch (error) { | ||
| console.error("[Chat History] Error in edit message:", error) | ||
| vscode.window.showErrorMessage( | ||
| `Error editing message: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| } | ||
| } else { | ||
| console.warn(`[Chat History] Message with timestamp ${messageTs} not found for editing`) | ||
| } | ||
| } | ||
| } | ||
|
|
||
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.
Good job updating the test expectations to match the new exact timestamp behavior. Consider adding a dedicated test case specifically for the function to ensure it handles edge cases like duplicate timestamps correctly.