-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: resolve chat message edit/delete duplication issues #7793
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
- 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
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback logic in handleEditMessageConfirm (lines 347-357) duplicates similar logic in removeMessagesThisAndSubsequent; consider refactoring to avoid duplication.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this error message more descriptive? For example: 'Cannot delete message with timestamp ${message.messageTs}: timestamp is missing'
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.
And translate pls
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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") |
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.
Mind translating this?
| } | ||
|
|
||
| if (typeof message.messageTs !== "number") { | ||
| await vscode.window.showErrorMessage("Cannot delete message: invalid timestamp") |
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.
Here too
- 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.
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.