-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix chat history truncation during message operations #7142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Changed findMessageIndices to use exact timestamp matching (ts === messageTs) - Removed 1-second buffer that was causing unintended message deletion - Updated test expectations to match correct behavior - Fixes #6932 where chat history was being truncated during message operations
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.
Reviewed my own code. Found it surprisingly coherent for something I wrote 3 minutes ago.
| const timeCutoff = messageTs - 1000 // 1 second buffer before the message | ||
| const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts && msg.ts >= timeCutoff) | ||
| // Use exact timestamp matching to prevent unintended message deletion | ||
| const messageIndex = currentCline.clineMessages.findIndex((msg: ClineMessage) => msg.ts === messageTs) |
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.
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?
|
|
||
| /** | ||
| * Shared utility to find message indices based on timestamp | ||
| * Shared utility to find message indices based on exact timestamp matching |
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 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?
| } | ||
|
|
||
| // Log deletion for debugging | ||
| const messagesToDelete = currentCline.clineMessages.length - messageIndex |
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.
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.
| expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([mockMessages[0], mockMessages[1]]) | ||
| // With exact timestamp matching, we find the message at index 3 (ts: 4000) | ||
| // and keep messages 0, 1, and 2 (all before timestamp 4000) | ||
| expect(mockCline.overwriteClineMessages).toHaveBeenCalledWith([ |
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.
Summary
This PR fixes issue #6932 where chat history was being truncated during message-related operations (edit/delete).
Problem
The
findMessageIndicesfunction inwebviewMessageHandler.tswas using a 1-second timestamp buffer when searching for messages to delete or edit. This caused unintended deletion of unrelated messages that happened to fall within the 1-second window, resulting in chat history truncation.Solution
findMessageIndicesto use exact timestamp matching (msg.ts === messageTs) instead of the 1-second bufferTesting
Related Issues
Fixes #6932
Important
Fixes chat history truncation by using exact timestamp matching for message operations in
webviewMessageHandler.ts.findMessageIndicesinwebviewMessageHandler.tsto use exact timestamp matching instead of a 1-second buffer.ClineProvider.spec.tsto reflect exact timestamp matching behavior.This description was created by
for 809be07. You can customize this summary. It will automatically update as commits are pushed.