feat(session): add undo functionality to session messages and context menu #8391
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Based on my search, I found several related PRs but they are not exact duplicates of PR #8391. Here are the potentially related PRs: Related but not duplicates:
These PRs address related undo/revert functionality in sessions but focus on different aspects (todo state, sidebar updates, file handling) rather than the specific new features in PR #8391 (message-level context menu UI, desktop shell mode fix). No exact duplicate PRs found for the combination of features in this PR (message-level undo with context menu UI and desktop shell mode fix). |
There was a problem hiding this comment.
Pull request overview
This PR adds undo/revert functionality to session messages through a right-click context menu, enhancing the user experience with desktop-friendly interactions.
Changes:
- Adds a context menu to user messages with "Undo from here" and "Copy message" actions
- Implements intelligent revert logic that keeps older messages but removes subsequent turns, or acts as classic undo for the latest message
- Adds an "undo" icon to the icon library
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| packages/ui/src/components/session-turn.tsx | Adds DropdownMenu context menu to user messages with revert and copy functionality, and adds onRevert prop to SessionTurn component |
| packages/ui/src/components/icon.tsx | Adds new "undo" icon SVG definition |
| packages/app/src/pages/session.tsx | Implements onRevert handler with logic to revert messages, restore prompts, and update active message state |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Message message={msg()} parts={parts()} /> | ||
| <DropdownMenu placement="bottom-start" gutter={0}> | ||
| <DropdownMenu.Trigger | ||
| as={(p: any) => ( |
There was a problem hiding this comment.
Using any type for the render prop parameter weakens type safety. Consider defining a proper type for the trigger props to maintain type safety and better IDE support.
| } | ||
|
|
||
| // Update active message to the one we reverted to or the one before | ||
| setActiveMessage(idx < msgs.length - 1 ? msgs[idx] : msgs[idx - 1]) |
There was a problem hiding this comment.
When reverting the first and only message in a session (when idx = 0 and msgs.length = 1), the condition idx < msgs.length - 1 evaluates to false, so targetID = messageID (the first message). After reversion, setActiveMessage(msgs[idx - 1]) evaluates to msgs[-1] which is undefined. This edge case should be handled to prevent setting an undefined active message.
| setActiveMessage(idx < msgs.length - 1 ? msgs[idx] : msgs[idx - 1]) | |
| const newActiveMessage = | |
| idx < msgs.length - 1 | |
| ? msgs[idx] // there is a next message; keep this one active | |
| : idx > 0 | |
| ? msgs[idx - 1] // no next message; go to previous | |
| : msgs[0] // single-message case; keep the only message | |
| setActiveMessage(newActiveMessage) |
| onRevert={async (messageID) => { | ||
| const sessionID = params.id | ||
| if (!sessionID) return | ||
| if (status()?.type !== "idle") { | ||
| await sdk.client.session.abort({ sessionID }).catch(() => {}) | ||
| } | ||
| // Find the message in the list | ||
| const msgs = userMessages() | ||
| const idx = msgs.findIndex((m) => m.id === messageID) | ||
| if (idx === -1) return | ||
|
|
||
| // If there's a next message, revert to that (keeping this message) | ||
| // If this is the last message, revert this message itself (classic undo) | ||
| const targetID = idx < msgs.length - 1 ? msgs[idx + 1].id : messageID | ||
|
|
||
| await sdk.client.session.revert({ sessionID, messageID: targetID }) | ||
|
|
||
| // Restore prompt from the target message | ||
| const parts = sync.data.part[targetID] | ||
| if (parts) { | ||
| const restored = extractPromptFromParts(parts, { directory: sdk.directory }) | ||
| prompt.set(restored) | ||
| } | ||
|
|
||
| // Update active message to the one we reverted to or the one before | ||
| setActiveMessage(idx < msgs.length - 1 ? msgs[idx] : msgs[idx - 1]) | ||
| }} |
There was a problem hiding this comment.
The logic in this handler is complex and duplicates similar logic from the existing session.undo command (lines 566-593). Consider extracting this revert logic into a shared helper function to reduce code duplication and improve maintainability. Both implementations should be kept in sync, and a shared function would prevent divergence.
| <Show when={props.onRevert}> | ||
| <DropdownMenu.Item onSelect={() => props.onRevert?.(msg().id)}> | ||
| <Icon name="undo" size="small" /> | ||
| <DropdownMenu.ItemLabel>Undo from here</DropdownMenu.ItemLabel> |
There was a problem hiding this comment.
The label "Undo from here" is inconsistent with the PR description and may be confusing. According to the description, this should be "Revert to here" for older messages or act as "Undo" for the latest message. The label should dynamically reflect this distinction to match user expectations, or use a consistent label like "Revert to here" as mentioned in the PR title and description.
| <DropdownMenu.ItemLabel>Undo from here</DropdownMenu.ItemLabel> | |
| <DropdownMenu.ItemLabel>Revert to here</DropdownMenu.ItemLabel> |
| onSelect={() => { | ||
| const text = parts() | ||
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | ||
| .join("") | ||
| navigator.clipboard.writeText(text) |
There was a problem hiding this comment.
The clipboard write operation lacks error handling. If the clipboard API fails (e.g., due to permissions or browser restrictions), the user won't receive any feedback. Consider adding error handling with a try-catch block and potentially showing a toast notification on success or failure.
| onSelect={() => { | |
| const text = parts() | |
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | |
| .join("") | |
| navigator.clipboard.writeText(text) | |
| onSelect={async () => { | |
| const text = parts() | |
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | |
| .join("") | |
| try { | |
| await navigator.clipboard.writeText(text) | |
| window.alert("Message copied to clipboard.") | |
| } catch (error) { | |
| console.error("Failed to copy message to clipboard:", error) | |
| window.alert("Failed to copy message to clipboard. Please try again.") | |
| } |
| <DropdownMenu placement="bottom-start" gutter={0}> | ||
| <DropdownMenu.Trigger | ||
| as={(p: any) => ( | ||
| <div | ||
| {...p} | ||
| onContextMenu={(e) => { | ||
| e.preventDefault() | ||
| p.onClick(e) | ||
| }} | ||
| > | ||
| <Message message={msg()} parts={parts()} /> | ||
| </div> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
The context menu is only triggered by right-click (onContextMenu), which may not be accessible to keyboard-only users. While the DropdownMenu component likely provides keyboard support through the underlying Kobalte library, users should be able to access this functionality without a mouse. Consider documenting the keyboard shortcut or ensuring there's an alternative way to access these actions via keyboard.
| <DropdownMenu placement="bottom-start" gutter={0}> | ||
| <DropdownMenu.Trigger | ||
| as={(p: any) => ( | ||
| <div | ||
| {...p} | ||
| onContextMenu={(e) => { | ||
| e.preventDefault() | ||
| p.onClick(e) | ||
| }} | ||
| > | ||
| <Message message={msg()} parts={parts()} /> | ||
| </div> | ||
| )} | ||
| /> | ||
| <DropdownMenu.Portal> | ||
| <DropdownMenu.Content> | ||
| <Show when={props.onRevert}> | ||
| <DropdownMenu.Item onSelect={() => props.onRevert?.(msg().id)}> | ||
| <Icon name="undo" size="small" /> | ||
| <DropdownMenu.ItemLabel>Undo from here</DropdownMenu.ItemLabel> | ||
| </DropdownMenu.Item> | ||
| </Show> | ||
| <DropdownMenu.Item | ||
| onSelect={() => { | ||
| const text = parts() | ||
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | ||
| .join("") | ||
| navigator.clipboard.writeText(text) | ||
| }} | ||
| > | ||
| <Icon name="copy" size="small" /> | ||
| <DropdownMenu.ItemLabel>Copy message</DropdownMenu.ItemLabel> | ||
| </DropdownMenu.Item> | ||
| </DropdownMenu.Content> | ||
| </DropdownMenu.Portal> | ||
| </DropdownMenu> |
There was a problem hiding this comment.
The context menu wraps the entire Message component, which could cause unexpected behavior if the Message component itself has interactive elements or handles click events. This could lead to event conflicts or prevent users from interacting with the message content normally. Consider adding a visual indicator (like a small icon/button) that the message has a context menu, or ensure that the Message component's interactive elements aren't affected by this wrapper.
| <DropdownMenu placement="bottom-start" gutter={0}> | |
| <DropdownMenu.Trigger | |
| as={(p: any) => ( | |
| <div | |
| {...p} | |
| onContextMenu={(e) => { | |
| e.preventDefault() | |
| p.onClick(e) | |
| }} | |
| > | |
| <Message message={msg()} parts={parts()} /> | |
| </div> | |
| )} | |
| /> | |
| <DropdownMenu.Portal> | |
| <DropdownMenu.Content> | |
| <Show when={props.onRevert}> | |
| <DropdownMenu.Item onSelect={() => props.onRevert?.(msg().id)}> | |
| <Icon name="undo" size="small" /> | |
| <DropdownMenu.ItemLabel>Undo from here</DropdownMenu.ItemLabel> | |
| </DropdownMenu.Item> | |
| </Show> | |
| <DropdownMenu.Item | |
| onSelect={() => { | |
| const text = parts() | |
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | |
| .join("") | |
| navigator.clipboard.writeText(text) | |
| }} | |
| > | |
| <Icon name="copy" size="small" /> | |
| <DropdownMenu.ItemLabel>Copy message</DropdownMenu.ItemLabel> | |
| </DropdownMenu.Item> | |
| </DropdownMenu.Content> | |
| </DropdownMenu.Portal> | |
| </DropdownMenu> | |
| <div data-slot="session-turn-message-with-menu"> | |
| <Message message={msg()} parts={parts()} /> | |
| <DropdownMenu placement="bottom-start" gutter={0}> | |
| <DropdownMenu.Trigger | |
| as={(p: any) => ( | |
| <button | |
| type="button" | |
| {...p} | |
| data-slot="session-turn-message-menu-trigger" | |
| onClick={(e) => { | |
| e.stopPropagation() | |
| p.onClick?.(e) | |
| }} | |
| onContextMenu={(e) => { | |
| e.preventDefault() | |
| e.stopPropagation() | |
| p.onClick?.(e) | |
| }} | |
| > | |
| <Icon name="chevron-grabber-vertical" size="small" /> | |
| </button> | |
| )} | |
| /> | |
| <DropdownMenu.Portal> | |
| <DropdownMenu.Content> | |
| <Show when={props.onRevert}> | |
| <DropdownMenu.Item onSelect={() => props.onRevert?.(msg().id)}> | |
| <Icon name="undo" size="small" /> | |
| <DropdownMenu.ItemLabel>Undo from here</DropdownMenu.ItemLabel> | |
| </DropdownMenu.Item> | |
| </Show> | |
| <DropdownMenu.Item | |
| onSelect={() => { | |
| const text = parts() | |
| .map((p) => (p?.type === "text" ? (p as TextPart).text : "")) | |
| .join("") | |
| navigator.clipboard.writeText(text) | |
| }} | |
| > | |
| <Icon name="copy" size="small" /> | |
| <DropdownMenu.ItemLabel>Copy message</DropdownMenu.ItemLabel> | |
| </DropdownMenu.Item> | |
| </DropdownMenu.Content> | |
| </DropdownMenu.Portal> | |
| </DropdownMenu> | |
| </div> |
| lastUserMessageID?: string | ||
| stepsExpanded?: boolean | ||
| onStepsExpandedToggle?: () => void | ||
| onUserInteracted?: () => void |
There was a problem hiding this comment.
The new onRevert prop lacks JSDoc documentation. Adding documentation would help other developers understand when and how this callback is invoked, what the messageID parameter represents, and the expected behavior.
| onUserInteracted?: () => void | |
| onUserInteracted?: () => void | |
| /** | |
| * Optional callback invoked when the user chooses to revert a message in this session. | |
| * | |
| * @param messageID - The identifier of the message to revert within the current session. | |
| */ |
| if (status()?.type !== "idle") { | ||
| await sdk.client.session.abort({ sessionID }).catch(() => {}) | ||
| } | ||
| // Find the message in the list | ||
| const msgs = userMessages() | ||
| const idx = msgs.findIndex((m) => m.id === messageID) | ||
| if (idx === -1) return | ||
|
|
||
| // If there's a next message, revert to that (keeping this message) | ||
| // If this is the last message, revert this message itself (classic undo) | ||
| const targetID = idx < msgs.length - 1 ? msgs[idx + 1].id : messageID | ||
|
|
||
| await sdk.client.session.revert({ sessionID, messageID: targetID }) |
There was a problem hiding this comment.
There's a potential race condition when aborting a session followed immediately by reverting. If the abort operation fails or takes time, the revert might execute on a session that's still running. While the catch block prevents the error from propagating, consider whether the revert should only proceed after confirming the abort succeeded, or handle the case where the session might still be processing.
|
First thoughts: I'd need to see some screenshots, and the |
|
Thanks for the feedback. I’ll address the points you mentioned and open a new PR once the changes are ready. Appreciate you taking the time to review it. |
2 participants
What does this PR do?
How did you verify your code works?