feat: edit message and delete #7391
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds edit and delete functionality for chat messages in the thread view, allowing users to modify conversation history and regenerate responses from edited messages. The changes also remove tooltip wrappers from the edit and delete dialog triggers.
Changes:
- Added
handleEditMessageandhandleDeleteMessagefunctions in the thread detail route that update both backend storage and UI state - Modified
MessageItemcomponent to display edit and delete buttons for both user and assistant messages - Removed
Tooltipcomponents fromEditMessageDialogandDeleteMessageDialogtriggers - Changed regenerate button visibility to only show on the last assistant message
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| web-app/src/routes/threads/$threadId.tsx | Implements handlers for editing and deleting messages, including state updates and regeneration logic |
| web-app/src/containers/dialogs/EditMessageDialog.tsx | Removes tooltip wrapper from the edit trigger button |
| web-app/src/containers/dialogs/DeleteMessageDialog.tsx | Removes tooltip wrapper from the delete trigger button |
| web-app/src/containers/MessageItem.tsx | Adds edit/delete buttons to both user and assistant messages, restricts regenerate to last message only |
Comments suppressed due to low confidence (1)
web-app/src/containers/MessageItem.tsx:417
- The regenerate button visibility logic has changed from checking only selectedModel and onRegenerate to also requiring isLastMessage. This means users can no longer regenerate from earlier messages in the conversation (they can only regenerate from the last assistant message). This is a significant behavioral change that restricts functionality. If this is intentional, consider whether edit+regenerate (which is now available via the edit button) adequately replaces the ability to regenerate from any message. The old behavior allowed regenerating from any assistant message.
{selectedModel && onRegenerate && !isStreaming && isLastMessage && (
<button
className="flex items-center gap-1 hover:text-accent transition-colors cursor-pointer group relative"
onClick={handleRegenerate}
title="Regenerate response"
>
<IconRefresh size={16} />
</button>
)}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| className="flex items-center gap-1 hover:text-accent transition-colors cursor-pointer group relative" | ||
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| setIsOpen(true) | ||
| } | ||
| }} | ||
| > | ||
| <IconTrash size={16} /> | ||
| </div> |
There was a problem hiding this comment.
Removing the Tooltip components reduces accessibility for users who rely on tooltips to understand button functionality. Consider adding an aria-label attribute to the trigger div (e.g., aria-label="Delete message") to maintain accessibility for screen reader users, especially since this is an icon-only button without a visible text label.
| // Update chat messages for UI | ||
| const updatedChatMessages = chatMessages.map((msg) => { | ||
| if (msg.id === messageId) { | ||
| return { | ||
| ...msg, | ||
| parts: [{ type: 'text' as const, text: newText }], | ||
| } | ||
| } | ||
| return msg | ||
| }) | ||
| setChatMessages(updatedChatMessages) |
There was a problem hiding this comment.
The handleEditMessage function has potential state synchronization issues. It updates both the backend messages (updateMessage) and the UI chatMessages (setChatMessages) separately, but these updates are not atomic. If a message deletion or regeneration happens concurrently, the states could become inconsistent. Additionally, the function reads chatMessages at line 646 but that value might be stale if the component hasn't re-rendered yet with the latest backend changes. Consider using a single source of truth or ensuring proper state synchronization between the backend messages and UI chatMessages.
| // Update chat messages for UI | |
| const updatedChatMessages = chatMessages.map((msg) => { | |
| if (msg.id === messageId) { | |
| return { | |
| ...msg, | |
| parts: [{ type: 'text' as const, text: newText }], | |
| } | |
| } | |
| return msg | |
| }) | |
| setChatMessages(updatedChatMessages) | |
| // Update chat messages for UI using functional update to avoid stale state | |
| setChatMessages((prevChatMessages) => | |
| prevChatMessages.map((msg) => { | |
| if (msg.id === messageId) { | |
| return { | |
| ...msg, | |
| parts: [{ type: 'text' as const, text: newText }], | |
| } | |
| } | |
| return msg | |
| }) | |
| ) |
| const handleDeleteMessage = useCallback( | ||
| (messageId: string) => { | ||
| deleteMessage(threadId, messageId) | ||
|
|
||
| // Update chat messages for UI | ||
| const updatedChatMessages = chatMessages.filter( | ||
| (msg) => msg.id !== messageId | ||
| ) | ||
| setChatMessages(updatedChatMessages) | ||
| }, | ||
| [threadId, deleteMessage, chatMessages, setChatMessages] | ||
| ) |
There was a problem hiding this comment.
The handleDeleteMessage function only updates the local state (backend via deleteMessage and UI via setChatMessages) but doesn't consider the implications for conversation continuity. If an assistant message is deleted, the conversation might become incoherent (a user message with no response), or if a user message is deleted, the following assistant message loses its context. Consider adding validation to prevent deletion of messages that would break conversation flow, or implement cascade deletion logic (e.g., when deleting a user message, also delete subsequent assistant responses until the next user message).
| {onEdit && !isStreaming && ( | ||
| <EditMessageDialog | ||
| message={getFullTextContent()} | ||
| onSave={handleEdit} | ||
| /> | ||
| )} | ||
|
|
There was a problem hiding this comment.
Allowing users to edit assistant messages (lines 398-403) is problematic because assistant messages represent AI-generated content that the user didn't create. Editing an assistant message and then regenerating from it could lead to confusion about what was AI-generated vs. user-modified. Consider restricting edit functionality to user messages only (role === 'user'), similar to how it's already done at lines 372-378. If editing assistant messages is intentionally allowed, this should be clearly documented with a use case justification.
| {onEdit && !isStreaming && ( | |
| <EditMessageDialog | |
| message={getFullTextContent()} | |
| onSave={handleEdit} | |
| /> | |
| )} |
| {onDelete && !isStreaming && ( | ||
| <DeleteMessageDialog onDelete={handleDelete} /> | ||
| )} | ||
|
|
There was a problem hiding this comment.
Allowing users to delete assistant messages (lines 405-407) could break conversation flow and context. If an assistant message is deleted but its corresponding user message remains, the conversation becomes incomplete. Consider either: (1) restricting delete to user messages only, or (2) implementing cascade deletion where deleting a user message also deletes all subsequent messages until the next user message, and deleting an assistant message deletes all messages from the preceding user message onwards. This would maintain conversation integrity.
| {onDelete && !isStreaming && ( | |
| <DeleteMessageDialog onDelete={handleDelete} /> | |
| )} |
| regenerate(messageId ? { messageId } : undefined) | ||
| } | ||
|
|
||
| // Handle edit message - updates the message and regenerates from it |
There was a problem hiding this comment.
The imageUrls parameter is prefixed with an underscore indicating it's intentionally unused, but the EditMessageDialog is designed to handle image editing (lines 25-26 in EditMessageDialog.tsx show imageUrls is used). This means edited images won't be persisted to the backend when a message is edited. Consider removing the underscore and implementing proper image handling in the updateMessage call, or document why images aren't being persisted during edits.
| // Handle edit message - updates the message and regenerates from it | |
| // Handle edit message - updates the message and regenerates from it. | |
| // | |
| // Note: EditMessageDialog can provide `imageUrls` when a message is edited, | |
| // but at the moment we only persist text changes here. Image additions, | |
| // removals, or modifications are intentionally not persisted yet. The | |
| // `_imageUrls` parameter is accepted to satisfy the dialog's interface and | |
| // reserved for future support of editing message images. |
| <div | ||
| className="flex outline-0 items-center gap-1 hover:text-accent transition-colors cursor-pointer group relative" | ||
| role="button" | ||
| tabIndex={0} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault() | ||
| setIsOpen(true) | ||
| } | ||
| }} | ||
| > | ||
| <IconPencil size={16} /> | ||
| </div> |
There was a problem hiding this comment.
Removing the Tooltip components reduces accessibility for users who rely on tooltips to understand button functionality. The CopyButton at line 16-28 in CopyButton.tsx shows text when copied but the edit/delete buttons now have no hover tooltip or accessible label. Consider adding aria-label attributes to the trigger divs (e.g., aria-label="Edit message") to maintain accessibility for screen reader users, especially since these are icon-only buttons without visible text labels.
Describe Your Changes
Fixes Issues
Self Checklist