-
Notifications
You must be signed in to change notification settings - Fork 7.1k
feat(session): add undo functionality to session messages and context menu #8391
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1177,6 +1177,33 @@ export default function Page() { | |||||||||||||||||
| onStepsExpandedToggle={() => | ||||||||||||||||||
| setStore("expanded", message.id, (open: boolean | undefined) => !open) | ||||||||||||||||||
| } | ||||||||||||||||||
| 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]) | ||||||||||||||||||
|
||||||||||||||||||
| 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) |
Copilot
AI
Jan 14, 2026
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 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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,8 @@ import { createStore } from "solid-js/store" | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { DateTime, DurationUnit, Interval } from "luxon" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { createAutoScroll } from "../hooks" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { DropdownMenu } from "./dropdown-menu" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function computeStatusFromPart(part: PartType | undefined): string | undefined { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!part) return undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -124,6 +126,7 @@ export function SessionTurn( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stepsExpanded?: boolean | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onStepsExpandedToggle?: () => void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onUserInteracted?: () => void | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. | |
| */ |
Copilot
AI
Jan 14, 2026
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.
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.
Copilot
AI
Jan 14, 2026
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 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.
Copilot
AI
Jan 14, 2026
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 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> |
Copilot
AI
Jan 14, 2026
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 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.") | |
| } |
Copilot
AI
Jan 14, 2026
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 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> |
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.
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.