fix: prevent chat history truncation during message operations #6934
Conversation
- Changed findMessageIndices to use exact timestamp matching instead of 1-second buffer - Added support for value field matching to handle message references correctly - Enhanced validation to prevent deletion of critical messages - Added proper error handling for edge cases when messages don't exist - Fixed issue where unrelated messages were being deleted due to timestamp proximity Fixes #6932
dosubot
bot
added
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| ) | ||
| // Validate that we're not deleting critical messages | ||
| if (messageIndex === 0) { | ||
| vscode.window.showWarningMessage("Cannot delete the first message in the conversation") |
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.
| vscode.window.showWarningMessage("Cannot delete the first message in the conversation") | |
| vscode.window.showWarningMessage(t("common:errors.cannot_delete_first_message")) |
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| await provider.contextProxy.setValue(key, value) | ||
|
|
||
| /** | ||
| * Shared utility to find message indices based on timestamp |
There was a problem hiding this comment.
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.
| const newApiHistory = currentCline.apiConversationHistory.slice(0, apiConversationHistoryIndex) | ||
| await currentCline.overwriteApiConversationHistory(newApiHistory) | ||
|
|
||
| console.log(`Removed ${originalApiLength - newApiHistory.length} API conversation messages`) |
There was a problem hiding this comment.
The logging could be more specific about what type of messages were removed. Consider:
| console.log(`Removed ${originalApiLength - newApiHistory.length} API conversation messages`) | |
| console.log(`Removed ${originalApiLength - newApiHistory.length} API conversation messages from history`) |
| console.log(`Removed ${originalApiLength - newApiHistory.length} API conversation messages`) | ||
| } | ||
|
|
||
| console.log(`Removed ${originalClineLength - newClineMessages.length} Cline messages`) |
There was a problem hiding this comment.
Similarly here, we could be more specific:
| console.log(`Removed ${originalClineLength - newClineMessages.length} Cline messages`) | |
| console.log(`Removed ${originalClineLength - newClineMessages.length} Cline messages from conversation`) |
| `Error deleting message: ${error instanceof Error ? error.message : String(error)}`, | ||
| ) | ||
| // Validate that we're not deleting critical messages | ||
| if (messageIndex === 0) { |
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?
| (msg: ApiMessage) => msg.ts && msg.ts >= timeCutoff, | ||
| // 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( |
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:
- Finding messages by value field
- Fallback to timestamp when value field doesn't match
- The precedence behavior when both could match
hannesrudolph
added
the
Issue/PR - Triage
|
This is most likely incorrect, the issue is not properly scoped yet |
Summary
This PR fixes issue #6932 where chat history was getting unexpectedly truncated during message deletion, restoration, and editing operations.
Problem
The root cause was in the
findMessageIndicesfunction inwebviewMessageHandler.ts, which was using a 1-second timestamp buffer (messageTs - 1000) when searching for messages. This could inadvertently match and delete unrelated messages that happened to be created within that 1-second window.Solution
findMessageIndicesto use exact timestamp matching instead of the 1-second buffervaluefield, which is used to reference related messagesTesting
Impact
This fix ensures that only the intended messages are deleted or edited, preventing accidental loss of conversation context.
Fixes #6932
Important
Fixes chat history truncation by updating message handling logic to use exact timestamp matching and adding validation to prevent critical message deletions.
findMessageIndicesinwebviewMessageHandler.tsnow uses exact timestamp matching, removing the 1-second buffer.valuefield for related messages.ClineProvider.spec.tsto reflect changes in message handling logic.This description was created by
for 87a7388. You can customize this summary. It will automatically update as commits are pushed.