From 9a609a4e444ba739d6fcd607769d68b679b0bcc5 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Mon, 21 Sep 2026 18:52:08 -0700 Subject: [PATCH 1/7] fix(web): show thread undo notice in the sidebar (#12972) --- apps/web/src/components/ChatView.tsx | 12 -- apps/web/src/components/Sidebar.tsx | 40 +---- .../src/components/sidebar/SidebarChrome.tsx | 2 + .../sidebar/SidebarThreadUndoNotice.tsx | 30 ++++ apps/web/src/components/ui/alert.tsx | 2 + .../src/hooks/showThreadUndoNotice.test.ts | 152 ++++++++++++++++++ apps/web/src/hooks/showThreadUndoNotice.ts | 106 ++++++++++++ apps/web/src/hooks/showUndoToast.test.ts | 127 --------------- apps/web/src/hooks/showUndoToast.ts | 87 ---------- apps/web/src/hooks/threadUndo.ts | 19 ++- apps/web/src/hooks/useThreadActions.ts | 47 ++---- .../src/hooks/useThreadActions.undo.test.ts | 71 ++++---- apps/web/src/routes/_chat.tsx | 14 ++ docs/user/keybindings.md | 9 +- 14 files changed, 378 insertions(+), 340 deletions(-) create mode 100644 apps/web/src/components/sidebar/SidebarThreadUndoNotice.tsx create mode 100644 apps/web/src/hooks/showThreadUndoNotice.test.ts create mode 100644 apps/web/src/hooks/showThreadUndoNotice.ts delete mode 100644 apps/web/src/hooks/showUndoToast.test.ts delete mode 100644 apps/web/src/hooks/showUndoToast.ts diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index c9540e4a0a8..981f6f2c04a 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -230,7 +230,6 @@ import { import { BranchToolbar, type BranchToolbarHandle } from "./BranchToolbar"; import { resolveShortcutCommand, shortcutLabelForCommand } from "../keybindings"; import { isEditableFocused } from "../lib/editableFocus"; -import { undoLatestThreadAction } from "../hooks/showUndoToast"; import ThreadTerminalDrawer from "./ThreadTerminalDrawer"; import { AlarmClockIcon, @@ -6731,17 +6730,6 @@ export default function ChatView(props: ChatViewProps) { return; } - if (command === "thread.undo") { - // Only claim the chord when there is an Undo to run; otherwise the - // page keeps its native behavior for the key. - if (event.repeat) return; - if (undoLatestThreadAction()) { - event.preventDefault(); - event.stopPropagation(); - } - return; - } - if (command === "thread.pin") { event.preventDefault(); event.stopPropagation(); diff --git a/apps/web/src/components/Sidebar.tsx b/apps/web/src/components/Sidebar.tsx index 23c55fe8ecc..6341b34b376 100644 --- a/apps/web/src/components/Sidebar.tsx +++ b/apps/web/src/components/Sidebar.tsx @@ -3077,9 +3077,7 @@ export default function Sidebar() { settlingThreadKeysRef.current.add(threadKey); try { const navigateAfterSettle = planForwardNavigation(threadKey, opts.coSettlingKeys); - const result = await settleThread(threadRef, { - undoToast: opts.coSettlingKeys === undefined, - }); + const result = await settleThread(threadRef); if (result._tag === "Failure") { // Never navigate away from a thread that did not settle. if (!isAtomCommandInterrupted(result)) { @@ -3731,9 +3729,7 @@ export default function Sidebar() { // Snoozing the open thread moves you forward, same as settle — // both park the thread you're done with for now. const navigateAfterSnooze = planForwardNavigation(threadKey, opts.coSnoozingKeys); - const result = await snoozeThread(threadRef, preset.snoozedUntil, { - undoToast: opts.coSnoozingKeys === undefined, - }); + const result = await snoozeThread(threadRef, preset.snoozedUntil); if (result._tag === "Failure") { // Never navigate away from a thread that did not snooze. return isAtomCommandInterrupted(result) @@ -3887,35 +3883,15 @@ export default function Sidebar() { outcome.status === "failure" ? [outcome.error] : [], ); - if (snoozedThreadRefs.length > 0) { - const snoozedCount = snoozedThreadRefs.length; - const failedCount = failures.length; - toastManager.add( - stackedThreadToast({ - type: failedCount > 0 ? "warning" : "success", - title: - failedCount > 0 - ? `Snoozed ${snoozedCount} of ${selectedThreads.length} threads` - : `Snoozed ${snoozedCount} thread${snoozedCount === 1 ? "" : "s"}`, - description: - failedCount > 0 - ? `${failedCount} thread${failedCount === 1 ? "" : "s"} couldn't be snoozed.` - : undefined, - timeout: 5_000, - actionProps: { - children: "Undo", - onClick: () => { - for (const threadRef of snoozedThreadRefs) attemptUnsnooze(threadRef); - }, - }, - }), - ); - } else if (failures.length > 0) { + if (failures.length > 0) { const firstError = failures[0]; toastManager.add( stackedThreadToast({ type: "error", - title: "Failed to snooze threads", + title: + snoozedThreadRefs.length > 0 + ? `Failed to snooze ${failures.length} thread${failures.length === 1 ? "" : "s"}` + : "Failed to snooze threads", description: firstError instanceof Error ? firstError.message : "An error occurred.", }), @@ -4019,7 +3995,6 @@ export default function Sidebar() { }, [ attemptSettle, - attemptSnooze, attemptUnpin, clearSelection, confirmThreadDelete, @@ -4028,7 +4003,6 @@ export default function Sidebar() { performSnooze, removeFromSelection, serverConfigs, - attemptUnsnooze, updateThreadMetadata, timestampFormat, ], diff --git a/apps/web/src/components/sidebar/SidebarChrome.tsx b/apps/web/src/components/sidebar/SidebarChrome.tsx index 22ebf1831e0..6433a1cba81 100644 --- a/apps/web/src/components/sidebar/SidebarChrome.tsx +++ b/apps/web/src/components/sidebar/SidebarChrome.tsx @@ -26,6 +26,7 @@ import { } from "../ui/sidebar"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { readPullRequestListPreferences } from "../pullRequest/pullRequestListPreferences"; +import { SidebarThreadUndoNotice } from "./SidebarThreadUndoNotice"; import { SidebarProviderUpdatePill } from "./SidebarProviderUpdatePill"; import { SidebarUpdateArchitectureWarning, SidebarUpdatePill } from "./SidebarUpdatePill"; import { PullRequestGlyph } from "~/components/pullRequest/pullRequestIcons"; @@ -221,6 +222,7 @@ export const SidebarUtilityMenu = memo(function SidebarUtilityMenu() { export const SidebarChromeFooter = memo(function SidebarChromeFooter() { return ( + diff --git a/apps/web/src/components/sidebar/SidebarThreadUndoNotice.tsx b/apps/web/src/components/sidebar/SidebarThreadUndoNotice.tsx new file mode 100644 index 00000000000..74f767f05d9 --- /dev/null +++ b/apps/web/src/components/sidebar/SidebarThreadUndoNotice.tsx @@ -0,0 +1,30 @@ +import { useAtomValue } from "@effect/atom-react"; + +import { undoLatestThreadAction, useThreadUndoNotice } from "../../hooks/showThreadUndoNotice"; +import { shortcutLabelForCommand } from "../../keybindings"; +import { primaryServerKeybindingsAtom } from "../../state/server"; +import { Alert, AlertDescription } from "../ui/alert"; +import { InlineButton } from "../ui/button"; + +export function SidebarThreadUndoNotice() { + const notice = useThreadUndoNotice((state) => state.notice); + const keybindings = useAtomValue(primaryServerKeybindingsAtom); + + if (!notice) return null; + const shortcut = shortcutLabelForCommand(keybindings, "thread.undo"); + + return ( + + + {notice.action} {notice.count} thread{notice.count === 1 ? "" : "s"},{" "} + + {shortcut ? `${shortcut} to undo` : "Undo"} + + + + ); +} diff --git a/apps/web/src/components/ui/alert.tsx b/apps/web/src/components/ui/alert.tsx index d071fd2dad5..e2a08066e6c 100644 --- a/apps/web/src/components/ui/alert.tsx +++ b/apps/web/src/components/ui/alert.tsx @@ -11,6 +11,8 @@ const alertVariants = cva("relative rounded-xl border px-3.5 py-3 text-card-fore variants: { variant: { default: "bg-transparent dark:bg-input/32 [&_svg]:text-muted-foreground", + sidebar: + "rounded-lg border-sidebar-border bg-sidebar-control-surface px-2 py-1.5 text-[11px] leading-4 [&_[data-slot=alert-description]]:block [&_[data-slot=alert-description]]:text-sidebar-muted-foreground", error: "border-error/32 bg-error-surface text-error-foreground [&_[data-slot=alert-description]]:text-error-foreground/80 [&_svg]:text-error", info: "border-info/32 bg-info/4 [&_svg]:text-info", diff --git a/apps/web/src/hooks/showThreadUndoNotice.test.ts b/apps/web/src/hooks/showThreadUndoNotice.test.ts new file mode 100644 index 00000000000..aceb70c21d9 --- /dev/null +++ b/apps/web/src/hooks/showThreadUndoNotice.test.ts @@ -0,0 +1,152 @@ +import { AsyncResult } from "effect/unstable/reactivity"; +import * as Cause from "effect/Cause"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"; + +import { toastManager } from "../components/ui/toast"; +import { + showThreadUndoNotice, + undoLatestThreadAction, + useThreadUndoNotice, +} from "./showThreadUndoNotice"; +import * as ThreadUndo from "./threadUndo"; + +beforeEach(() => vi.useFakeTimers()); +afterEach(() => { + vi.runAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +function setup() { + const add = vi.spyOn(toastManager, "add").mockReturnValue("error-toast"); + const undo = vi.fn(async () => AsyncResult.success(undefined)); + const claim = ThreadUndo.begin("pin", "env/thread"); + const options = { action: "Unpinned" as const, failureTitle: "Restore failed", undo, claim }; + return { add, undo, claim, options }; +} + +function notice() { + const value = useThreadUndoNotice.getState().notice; + if (!value) throw new Error("Undo notice is missing"); + return value; +} + +describe("thread undo notice", () => { + it("aggregates consecutive actions without success toasts and restores the group once", async () => { + const { add, undo, options } = setup(); + showThreadUndoNotice({ + ...options, + action: "Settled", + claim: ThreadUndo.begin("settle", "env/a"), + }); + showThreadUndoNotice({ + ...options, + action: "Settled", + claim: ThreadUndo.begin("settle", "env/b"), + }); + expect(notice()).toMatchObject({ action: "Settled", count: 2 }); + expect(add).not.toHaveBeenCalled(); + const group = notice(); + await group.undo(); + await group.undo(); + expect(undo).toHaveBeenCalledTimes(2); + expect(undoLatestThreadAction()).toBe(false); + }); + + it("drops invalidated claims immediately and rejects a captured stale undo", async () => { + const { undo, options } = setup(); + showThreadUndoNotice(options); + const stale = notice(); + ThreadUndo.invalidate("pin", "env/thread"); + expect(useThreadUndoNotice.getState().notice).toBeNull(); + showThreadUndoNotice({ ...options, claim: ThreadUndo.begin("pin", "env/thread") }); + await stale.undo(); + expect(undo).not.toHaveBeenCalled(); + await notice().undo(); + expect(undo).toHaveBeenCalledOnce(); + }); + + it("does not show a notice for a late completion after a newer action", () => { + const { add, options } = setup(); + ThreadUndo.invalidate("pin", "env/thread"); + showThreadUndoNotice(options); + expect(useThreadUndoNotice.getState().notice).toBeNull(); + expect(add).not.toHaveBeenCalled(); + }); + + it("keeps the group available until five seconds after the latest action", async () => { + const { undo, claim, options } = setup(); + showThreadUndoNotice(options); + vi.advanceTimersByTime(4_000); + const second = ThreadUndo.begin("pin", "env/second"); + showThreadUndoNotice({ ...options, claim: second }); + const group = notice(); + vi.advanceTimersByTime(4_999); + expect(notice().count).toBe(2); + vi.advanceTimersByTime(1); + expect(useThreadUndoNotice.getState().notice).toBeNull(); + expect(claim.isCurrent()).toBe(false); + expect(second.isCurrent()).toBe(false); + await group.undo(); + expect(undo).not.toHaveBeenCalled(); + }); + + it("reports a failed restore and releases its claim", async () => { + const { add, claim, options } = setup(); + showThreadUndoNotice({ + ...options, + undo: async () => AsyncResult.failure(Cause.fail(new Error("offline"))), + }); + await notice().undo(); + expect(add).toHaveBeenLastCalledWith( + expect.objectContaining({ type: "error", title: "Restore failed", description: "offline" }), + ); + expect(claim.isCurrent()).toBe(false); + }); + + it("reports a rejected restore promise", async () => { + const { add, options } = setup(); + showThreadUndoNotice({ + ...options, + undo: async () => { + throw new Error("disconnected"); + }, + }); + await notice().undo(); + expect(add).toHaveBeenLastCalledWith( + expect.objectContaining({ type: "error", description: "disconnected" }), + ); + }); + + it("does not report interrupted restores as errors", async () => { + const { add, options } = setup(); + showThreadUndoNotice({ ...options, undo: async () => AsyncResult.failure(Cause.interrupt()) }); + await notice().undo(); + expect(add).not.toHaveBeenCalled(); + }); + + it("undoes the latest kind first, then reveals the preceding group", async () => { + const { options } = setup(); + const older = vi.fn(async () => AsyncResult.success(undefined)); + const newer = vi.fn(async () => AsyncResult.success(undefined)); + showThreadUndoNotice({ + ...options, + action: "Settled", + undo: older, + claim: ThreadUndo.begin("settle", "env/a"), + }); + showThreadUndoNotice({ + ...options, + action: "Snoozed", + undo: newer, + claim: ThreadUndo.begin("snooze", "env/b"), + }); + expect(undoLatestThreadAction()).toBe(true); + expect(newer).toHaveBeenCalledOnce(); + expect(older).not.toHaveBeenCalled(); + expect(notice().action).toBe("Settled"); + await notice().undo(); + expect(older).toHaveBeenCalledOnce(); + expect(undoLatestThreadAction()).toBe(false); + }); +}); diff --git a/apps/web/src/hooks/showThreadUndoNotice.ts b/apps/web/src/hooks/showThreadUndoNotice.ts new file mode 100644 index 00000000000..ea296219487 --- /dev/null +++ b/apps/web/src/hooks/showThreadUndoNotice.ts @@ -0,0 +1,106 @@ +import { + type AtomCommandResult, + isAtomCommandInterrupted, + squashAtomCommandFailure, +} from "@t3tools/client-runtime/state/runtime"; +import { create } from "zustand"; + +import { stackedThreadToast, toastManager } from "../components/ui/toast"; +import * as ThreadUndo from "./threadUndo"; + +type UndoOptions = { + action: "Settled" | "Snoozed" | "Unpinned" | "Archived"; + undo: () => Promise>; + failureTitle: string; + claim: ReturnType; +}; + +type UndoNotice = { + action: UndoOptions["action"]; + count: number; + undo: () => Promise; +}; + +export const useThreadUndoNotice = create<{ notice: UndoNotice | null }>(() => ({ notice: null })); + +// Shared across sidebar, header and menu actions. Consecutive actions of the +// same kind share one notice and can be restored together. +let liveUndos: UndoOptions[] = []; +let expiry: ReturnType | undefined; + +function refreshNotice() { + liveUndos = liveUndos.filter(({ claim }) => claim.isCurrent()); + const latest = liveUndos.at(-1); + if (!latest) { + clearTimeout(expiry); + useThreadUndoNotice.setState({ notice: null }); + return; + } + const group: UndoOptions[] = []; + for (let index = liveUndos.length - 1; index >= 0; index--) { + const entry = liveUndos[index]!; + if (entry.action !== latest.action) break; + group.push(entry); + } + useThreadUndoNotice.setState({ + notice: { + action: latest.action, + count: group.length, + undo: async () => { + const current = group.filter( + (entry) => liveUndos.includes(entry) && entry.claim.isCurrent(), + ); + liveUndos = liveUndos.filter((entry) => !group.includes(entry)); + // Consume every claim before awaiting, so repeated clicks or shortcuts + // cannot restore the same group twice. + for (const entry of current) entry.claim.finish(); + refreshNotice(); + await Promise.all( + current.map(async ({ undo, failureTitle }) => { + const reportFailure = (error: unknown) => { + toastManager.add( + stackedThreadToast({ + type: "error", + title: failureTitle, + description: error instanceof Error ? error.message : "An error occurred.", + }), + ); + }; + try { + const result = await undo(); + if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { + reportFailure(squashAtomCommandFailure(result)); + } + } catch (error) { + reportFailure(error); + } + }), + ); + }, + }, + }); +} + +ThreadUndo.subscribe(refreshNotice); + +/** Runs the group displayed in the sidebar; false when nothing is left to undo. */ +export function undoLatestThreadAction(): boolean { + const notice = useThreadUndoNotice.getState().notice; + if (!notice) return false; + void notice.undo(); + return true; +} + +/** Shows one compact confirmation for the currently undoable thread actions. */ +export function showThreadUndoNotice(options: UndoOptions) { + if (!options.claim.isCurrent()) return; + liveUndos.push(options); + refreshNotice(); + clearTimeout(expiry); + expiry = setTimeout(() => { + const expired = liveUndos; + liveUndos = []; + for (const { claim } of expired) claim.finish(); + refreshNotice(); + }, 5_000); +} diff --git a/apps/web/src/hooks/showUndoToast.test.ts b/apps/web/src/hooks/showUndoToast.test.ts deleted file mode 100644 index ced0b2bc550..00000000000 --- a/apps/web/src/hooks/showUndoToast.test.ts +++ /dev/null @@ -1,127 +0,0 @@ -import { AsyncResult } from "effect/unstable/reactivity"; -import * as Cause from "effect/Cause"; -import { afterEach, describe, expect, it, vi } from "vite-plus/test"; - -import { toastManager } from "../components/ui/toast"; -import { showUndoToast, undoLatestThreadAction } from "./showUndoToast"; -import * as ThreadUndo from "./threadUndo"; - -afterEach(() => vi.restoreAllMocks()); - -function setup() { - const add = vi.spyOn(toastManager, "add").mockReturnValue("undo-toast"); - const close = vi.spyOn(toastManager, "close").mockImplementation(() => {}); - const undo = vi.fn(async () => AsyncResult.success(undefined)); - const claim = ThreadUndo.begin("pin", "env/thread"); - const options = { - title: "Thread unpinned", - description: "Thread", - failureTitle: "Restore failed", - undo, - claim, - }; - return { add, close, undo, claim, options }; -} - -function click(add: ReturnType["add"], index = 0) { - const handler = add.mock.calls[index]?.[0].actionProps?.onClick; - if (!handler) throw new Error("Undo action is missing"); - return handler({} as Parameters[0]); -} - -describe("showUndoToast", () => { - it("ignores a stale toast and lets the latest action run only once", async () => { - const { add, close, undo, options } = setup(); - showUndoToast(options); - ThreadUndo.invalidate("pin", "env/thread"); - showUndoToast({ ...options, claim: ThreadUndo.begin("pin", "env/thread") }); - await click(add); - expect(undo).not.toHaveBeenCalled(); - await click(add, 1); - await click(add, 1); - expect(undo).toHaveBeenCalledOnce(); - expect(close).toHaveBeenCalledExactlyOnceWith("undo-toast"); - }); - - it("releases the claim on close and rejects a later click", async () => { - const { add, undo, claim, options } = setup(); - showUndoToast(options); - add.mock.calls[0]?.[0].onClose?.(); - expect(claim.isCurrent()).toBe(false); - await click(add); - expect(undo).not.toHaveBeenCalled(); - }); - - it("does not show a toast for a late completion after a newer action", () => { - const { add, options } = setup(); - ThreadUndo.invalidate("pin", "env/thread"); - showUndoToast(options); - expect(add).not.toHaveBeenCalled(); - }); - - it("reports a failed restore and releases its claim", async () => { - const { add, claim, options } = setup(); - showUndoToast({ - ...options, - undo: async () => AsyncResult.failure(Cause.fail(new Error("offline"))), - }); - await click(add); - expect(add).toHaveBeenLastCalledWith( - expect.objectContaining({ type: "error", title: "Restore failed", description: "offline" }), - ); - expect(claim.isCurrent()).toBe(false); - }); - - it("reports a rejected restore promise", async () => { - const { add, options } = setup(); - showUndoToast({ - ...options, - undo: async () => { - throw new Error("disconnected"); - }, - }); - await click(add); - expect(add).toHaveBeenLastCalledWith( - expect.objectContaining({ type: "error", description: "disconnected" }), - ); - }); - - it("does not report interrupted restores as errors", async () => { - const { add, options } = setup(); - showUndoToast({ ...options, undo: async () => AsyncResult.failure(Cause.interrupt()) }); - await click(add); - expect(add).toHaveBeenCalledOnce(); - }); -}); - -describe("undoLatestThreadAction", () => { - it("runs the newest live Undo once and then reports nothing to undo", () => { - const { options } = setup(); - const older = vi.fn(async () => AsyncResult.success(undefined)); - const newer = vi.fn(async () => AsyncResult.success(undefined)); - showUndoToast({ ...options, undo: older, claim: ThreadUndo.begin("settle", "env/a") }); - showUndoToast({ ...options, undo: newer, claim: ThreadUndo.begin("snooze", "env/b") }); - expect(undoLatestThreadAction()).toBe(true); - expect(newer).toHaveBeenCalledOnce(); - expect(older).not.toHaveBeenCalled(); - expect(undoLatestThreadAction()).toBe(true); - expect(older).toHaveBeenCalledOnce(); - expect(undoLatestThreadAction()).toBe(false); - }); - - it("skips a superseded toast and a closed toast", () => { - const { add, options } = setup(); - const superseded = vi.fn(async () => AsyncResult.success(undefined)); - const closed = vi.fn(async () => AsyncResult.success(undefined)); - const live = vi.fn(async () => AsyncResult.success(undefined)); - showUndoToast({ ...options, undo: live, claim: ThreadUndo.begin("archive", "env/live") }); - showUndoToast({ ...options, undo: closed, claim: ThreadUndo.begin("archive", "env/closed") }); - add.mock.calls[1]?.[0].onClose?.(); - showUndoToast({ ...options, undo: superseded, claim: ThreadUndo.begin("pin", "env/stale") }); - ThreadUndo.invalidate("pin", "env/stale"); - expect(undoLatestThreadAction()).toBe(true); - expect(superseded).not.toHaveBeenCalled(); - expect(closed).not.toHaveBeenCalled(); - expect(live).toHaveBeenCalledOnce(); - }); -}); diff --git a/apps/web/src/hooks/showUndoToast.ts b/apps/web/src/hooks/showUndoToast.ts deleted file mode 100644 index 14d532411e7..00000000000 --- a/apps/web/src/hooks/showUndoToast.ts +++ /dev/null @@ -1,87 +0,0 @@ -import { - type AtomCommandResult, - isAtomCommandInterrupted, - squashAtomCommandFailure, -} from "@t3tools/client-runtime/state/runtime"; - -import { stackedThreadToast, toastManager } from "../components/ui/toast"; -import type * as ThreadUndo from "./threadUndo"; - -// Undo toasts still on screen, oldest first, so the `thread.undo` shortcut -// mirrors the newest toast's button without knowing which action it was. -const liveUndos: Array<() => Promise | null> = []; - -/** Runs the newest Undo whose claim still holds; false when nothing is left to undo. */ -export function undoLatestThreadAction(): boolean { - // A superseded entry drops itself when tried, so keep going until one - // runs or the list is empty. - while (liveUndos.length > 0) { - if (liveUndos[liveUndos.length - 1]?.() !== null) return true; - } - return false; -} - -/** Shows a single-use Undo while its thread action still owns the claim. */ -export function showUndoToast({ - title, - description, - undo, - failureTitle, - claim, -}: { - title: string; - description: string | undefined; - undo: () => Promise>; - failureTitle: string; - claim: ReturnType; -}) { - if (!claim.isCurrent()) return; - let undoStarted = false; - let toastId: string | undefined; - const reportFailure = (error: unknown) => { - toastManager.add( - stackedThreadToast({ - type: "error", - title: failureTitle, - description: error instanceof Error ? error.message : "An error occurred.", - }), - ); - }; - const forget = () => { - const index = liveUndos.indexOf(run); - if (index !== -1) liveUndos.splice(index, 1); - }; - const run = () => { - forget(); - if (undoStarted || !claim.isCurrent()) return null; - undoStarted = true; - claim.finish(); - if (toastId !== undefined) toastManager.close(toastId); - return undo() - .then((result) => { - if (result._tag === "Failure" && !isAtomCommandInterrupted(result)) { - reportFailure(squashAtomCommandFailure(result)); - } - }) - .catch(reportFailure); - }; - liveUndos.push(run); - toastId = toastManager.add({ - ...stackedThreadToast({ - type: "success", - title, - description, - timeout: 5_000, - actionProps: { - children: "Undo", - onClick: async () => { - await run(); - }, - }, - }), - onClose: () => { - claim.finish(); - forget(); - }, - }); -} diff --git a/apps/web/src/hooks/threadUndo.ts b/apps/web/src/hooks/threadUndo.ts index a9e5c179fc8..29d05b51efd 100644 --- a/apps/web/src/hooks/threadUndo.ts +++ b/apps/web/src/hooks/threadUndo.ts @@ -1,16 +1,32 @@ // Shared across hook instances so sidebar, header and menu actions invalidate each other. const currentActions = new Map(); +const listeners = new Set<() => void>(); + +export function subscribe(listener: () => void) { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +function notify() { + for (const listener of listeners) listener(); +} /** Claims one kind of thread action; a later claim of that kind expires its Undo. */ export function begin(kind: string, threadKey: string) { const key = JSON.stringify([kind, threadKey]); const token = Symbol(); currentActions.set(key, token); + notify(); const isCurrent = () => currentActions.get(key) === token; return { isCurrent, finish: () => { - if (isCurrent()) currentActions.delete(key); + if (isCurrent()) { + currentActions.delete(key); + notify(); + } }, }; } @@ -18,4 +34,5 @@ export function begin(kind: string, threadKey: string) { /** Expires only this action kind, leaving unrelated thread actions intact. */ export function invalidate(kind: string, threadKey: string) { currentActions.delete(JSON.stringify([kind, threadKey])); + notify(); } diff --git a/apps/web/src/hooks/useThreadActions.ts b/apps/web/src/hooks/useThreadActions.ts index 40a06ba9676..0e549109922 100644 --- a/apps/web/src/hooks/useThreadActions.ts +++ b/apps/web/src/hooks/useThreadActions.ts @@ -15,7 +15,6 @@ import { useRouter } from "@tanstack/react-router"; import { useCallback, useMemo, useRef } from "react"; import { getFallbackThreadIdAfterDelete, pinOrderKeyBetween } from "../components/Sidebar.logic"; -import { snoozeWakeDescription } from "../components/Sidebar.snooze"; import { useComposerDraftStore } from "../composerDraftStore"; import { terminalEnvironment } from "../state/terminal"; import { appAtomRegistry } from "../rpc/atomRegistry"; @@ -44,7 +43,7 @@ import { formatWorktreePathForDisplay, getOrphanedWorktreePathForThread } from " import { stackedThreadToast, toastManager } from "../components/ui/toast"; import { useClientSettings } from "./useSettings"; import * as ThreadUndo from "./threadUndo"; -import { showUndoToast } from "./showUndoToast"; +import { showThreadUndoNotice } from "./showThreadUndoNotice"; import { useAtomCommand } from "../state/use-atom-command"; export class ThreadArchiveBlockedError extends Schema.TaggedError()( @@ -223,7 +222,6 @@ export function useThreadActions() { const sidebarThreadSortOrder = useClientSettings((settings) => settings.sidebarThreadSortOrder); const confirmThreadDelete = useClientSettings((settings) => settings.confirmThreadDelete); const confirmThreadUnpin = useClientSettings((settings) => settings.confirmThreadUnpin); - const timestampFormat = useClientSettings((settings) => settings.timestampFormat); const clearComposerDraftForThread = useComposerDraftStore((store) => store.clearDraftThread); const clearProjectDraftThreadById = useComposerDraftStore( (store) => store.clearProjectDraftThreadById, @@ -313,9 +311,8 @@ export function useThreadActions() { } refreshArchivedThreadsForEnvironment(threadRef.environmentId); opts.onArchived?.(); - showUndoToast({ - title: "Thread archived", - description: thread.title, + showThreadUndoNotice({ + action: "Archived", claim: action, // Undo also brings the reader back when archiving moved them to a draft. undo: () => unarchiveThread(threadRef, { navigate: shouldNavigateToDraft }), @@ -613,9 +610,8 @@ export function useThreadActions() { input: { threadId: target.threadId }, }); if (result._tag === "Success" && action.isCurrent()) { - showUndoToast({ - title: "Thread unpinned", - description: thread?.title, + showThreadUndoNotice({ + action: "Unpinned", claim: action, undo: () => pinThread(target, orderKey === undefined ? {} : { orderKey }), failureTitle: "Failed to undo unpin", @@ -629,11 +625,7 @@ export function useThreadActions() { ); const settleThread = useCallback( - async ( - target: ScopedThreadRef, - // Batch callers settle a selection at once and stay silent as before. - opts: { undoToast?: boolean } = {}, - ) => { + async (target: ScopedThreadRef) => { // Version skew: never send the command to a server that predates it — // the raw protocol rejection would read as a random failure. if (!readEnvironmentSupportsSettlement(target.environmentId)) { @@ -671,13 +663,8 @@ export function useThreadActions() { if (wokeAt !== null) { markThreadVisited(scopedThreadKey(target), wokeAt); } - if (opts.undoToast === false) { - action.finish(); - return result; - } - showUndoToast({ - title: "Thread settled", - description: resolved?.thread.title, + showThreadUndoNotice({ + action: "Settled", claim: action, undo: async () => { const unsettled = await unsettleThread(target); @@ -797,12 +784,7 @@ export function useThreadActions() { ); const snoozeThread = useCallback( - async ( - target: ScopedThreadRef, - snoozedUntil: string, - // Batch callers report one toast for the whole selection instead. - opts: { undoToast?: boolean } = {}, - ) => { + async (target: ScopedThreadRef, snoozedUntil: string) => { // Version skew: never send the command to a server that predates it. if (!readEnvironmentSupportsSnooze(target.environmentId)) { return AsyncResult.failure( @@ -833,21 +815,20 @@ export function useThreadActions() { environmentId: target.environmentId, input: { threadId: target.threadId, snoozedUntil }, }); - if (result._tag !== "Success" || opts.undoToast === false) { + if (result._tag !== "Success") { action.finish(); return result; } - // Snooze hides the row, so the toast is the only confirmation. - showUndoToast({ - title: `Snoozed until ${snoozeWakeDescription(snoozedUntil, new Date(), timestampFormat)}`, - description: resolved?.thread.title, + // Snooze hides the row, so keep its confirmation in the sidebar. + showThreadUndoNotice({ + action: "Snoozed", claim: action, undo: () => unsnoozeThread(target), failureTitle: "Failed to wake thread", }); return result; }, - [resolveThreadTarget, snoozeThreadMutation, timestampFormat, unsnoozeThread], + [resolveThreadTarget, snoozeThreadMutation, unsnoozeThread], ); const confirmAndDeleteThread = useCallback( diff --git a/apps/web/src/hooks/useThreadActions.undo.test.ts b/apps/web/src/hooks/useThreadActions.undo.test.ts index eacffd146e2..6e61b298be5 100644 --- a/apps/web/src/hooks/useThreadActions.undo.test.ts +++ b/apps/web/src/hooks/useThreadActions.undo.test.ts @@ -4,6 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test" import { useThreadActions } from "./useThreadActions"; import { threadEnvironment } from "../state/threads"; import { toastManager } from "../components/ui/toast"; +import { useThreadUndoNotice } from "./showThreadUndoNotice"; const commands = vi.hoisted(() => ({ pin: vi.fn(), @@ -78,18 +79,14 @@ const target = { environmentId: EnvironmentId.make("undo-env"), threadId: ThreadId.make("thread"), }; -const event = {} as Parameters["onClick"]>>[0]; - -function undoOf( - add: { mock: { calls: Array<[Parameters[0]]> } }, - index: number, -) { - const onClick = add.mock.calls[index]?.[0].actionProps?.onClick; - expect(onClick).toBeTypeOf("function"); - return () => onClick?.(event); +function currentUndo() { + const notice = useThreadUndoNotice.getState().notice; + expect(notice).not.toBeNull(); + return notice!.undo; } beforeEach(() => { + vi.useFakeTimers(); for (const command of Object.values(commands)) { command.mockReset().mockResolvedValue({ _tag: "Success", value: undefined }); } @@ -98,19 +95,21 @@ beforeEach(() => { threadShell.pinnedAt = null; threadShell.snoozedUntil = null; }); -afterEach(() => vi.restoreAllMocks()); +afterEach(() => { + vi.runAllTimers(); + vi.useRealTimers(); + vi.restoreAllMocks(); +}); describe("unpin Undo", () => { - it("ignores an old toast across hook instances and still restores the latest unpin", async () => { - const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); + it("ignores an old notice across hook instances and still restores the latest unpin", async () => { const sidebar = useThreadActions(); const header = useThreadActions(); await sidebar.unpinThread(target); - const staleUndo = undoOf(add, 0); + const staleUndo = currentUndo(); await header.pinThread(target, { orderKey: "a1" }); await header.unpinThread(target); - const latestUndo = undoOf(add, 1); + const latestUndo = currentUndo(); await staleUndo(); expect(commands.pin).toHaveBeenCalledTimes(1); await latestUndo(); @@ -127,15 +126,15 @@ describe("unpin Undo", () => { describe("archive Undo", () => { it("unarchives and returns to the thread when archiving left it", async () => { const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); router.state.matches[0]!.params = { environmentId: target.environmentId, threadId: target.threadId, }; const actions = useThreadActions(); await actions.archiveThread(target); - expect(add).toHaveBeenCalledWith(expect.objectContaining({ title: "Thread archived" })); - await undoOf(add, 0)(); + expect(useThreadUndoNotice.getState().notice).toMatchObject({ action: "Archived", count: 1 }); + expect(add).not.toHaveBeenCalled(); + await currentUndo()(); expect(commands.unarchive).toHaveBeenCalledExactlyOnceWith({ environmentId: target.environmentId, input: { threadId: target.threadId }, @@ -149,11 +148,9 @@ describe("archive Undo", () => { }); it("stays put when the archived thread was not open", async () => { - const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); const actions = useThreadActions(); await actions.archiveThread(target); - await undoOf(add, 0)(); + await currentUndo()(); expect(commands.unarchive).toHaveBeenCalledOnce(); expect(router.navigate).not.toHaveBeenCalled(); }); @@ -167,27 +164,25 @@ describe("archive Undo", () => { }); describe("settle and snooze Undo", () => { - it("un-settles from the toast and expires the Undo after a manual un-settle", async () => { + it("un-settles from the notice and expires the Undo after a manual un-settle", async () => { const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); const actions = useThreadActions(); await actions.settleThread(target); - expect(add).toHaveBeenCalledWith(expect.objectContaining({ title: "Thread settled" })); - const undo = undoOf(add, 0); + expect(useThreadUndoNotice.getState().notice).toMatchObject({ action: "Settled", count: 1 }); + expect(add).not.toHaveBeenCalled(); + const undo = currentUndo(); await actions.unsettleThread(target); await undo(); expect(commands.unsettle).toHaveBeenCalledOnce(); }); it("re-pins and re-snoozes a thread that settling had cleared", async () => { - const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); const snoozedUntil = "2030-01-01T09:00:00.000Z"; threadShell.pinnedAt = "2026-01-01T00:00:00.000Z"; threadShell.snoozedUntil = snoozedUntil; const actions = useThreadActions(); await actions.settleThread(target); - await undoOf(add, 0)(); + await currentUndo()(); expect(commands.unsettle).toHaveBeenCalledOnce(); expect(commands.pin).toHaveBeenCalledExactlyOnceWith({ environmentId: target.environmentId, @@ -200,31 +195,21 @@ describe("settle and snooze Undo", () => { }); it("expires an older unpin Undo when the thread is settled", async () => { - const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); const actions = useThreadActions(); await actions.unpinThread(target); - const staleUnpinUndo = undoOf(add, 0); + const staleUnpinUndo = currentUndo(); await actions.settleThread(target); await staleUnpinUndo(); expect(commands.pin).not.toHaveBeenCalled(); }); - it("stays silent for batch settles", async () => { + it("wakes the thread from the snooze notice", async () => { const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - await useThreadActions().settleThread(target, { undoToast: false }); - expect(add).not.toHaveBeenCalled(); - }); - - it("wakes the thread from the snooze toast", async () => { - const add = vi.spyOn(toastManager, "add").mockReturnValue("toast"); - vi.spyOn(toastManager, "close").mockImplementation(() => {}); const actions = useThreadActions(); await actions.snoozeThread(target, new Date(Date.now() + 60_000).toISOString()); - expect(add).toHaveBeenCalledWith( - expect.objectContaining({ title: expect.stringMatching(/^Snoozed until /) }), - ); - await undoOf(add, 0)(); + expect(useThreadUndoNotice.getState().notice).toMatchObject({ action: "Snoozed", count: 1 }); + expect(add).not.toHaveBeenCalled(); + await currentUndo()(); expect(commands.unsnooze).toHaveBeenCalledExactlyOnceWith({ environmentId: target.environmentId, input: { threadId: target.threadId, reason: "user" }, diff --git a/apps/web/src/routes/_chat.tsx b/apps/web/src/routes/_chat.tsx index 980177004da..5b5b4888269 100644 --- a/apps/web/src/routes/_chat.tsx +++ b/apps/web/src/routes/_chat.tsx @@ -16,6 +16,9 @@ import { useHandleNewThread } from "../hooks/useHandleNewThread"; import { startNewThreadFromContext } from "../lib/chatThreadActions"; import { isPreviewFocused } from "../lib/previewFocus"; import { isTerminalFocused } from "../lib/terminalFocus"; +import { isEditableFocused } from "../lib/editableFocus"; +import { isModelPickerOpen } from "../modelPickerVisibility"; +import { undoLatestThreadAction } from "../hooks/showThreadUndoNotice"; import { resolveShortcutCommand } from "../keybindings"; import { selectThreadTerminalUiState, useTerminalUiStateStore } from "../terminalUiStateStore"; import { isPreviewSupportedInRuntime } from "../previewStateStore"; @@ -66,6 +69,8 @@ function ChatRouteGlobalShortcuts() { terminalOpen, previewFocus: isPreviewFocused(), previewOpen, + editableFocus: isEditableFocused(event.target), + modelPickerOpen: isModelPickerOpen(), }, }); @@ -73,6 +78,15 @@ function ChatRouteGlobalShortcuts() { return; } + if (command === "thread.undo") { + if (event.repeat || isModelPickerOpen()) return; + if (undoLatestThreadAction()) { + event.preventDefault(); + event.stopPropagation(); + } + return; + } + if (event.key === "Escape" && selectedThreadKeysSize > 0) { event.preventDefault(); clearSelection(); diff --git a/docs/user/keybindings.md b/docs/user/keybindings.md index 78c792d2985..548b03fd1a7 100644 --- a/docs/user/keybindings.md +++ b/docs/user/keybindings.md @@ -111,10 +111,11 @@ a shortcut. `thread.stop` interrupts the running turn in the focused thread. It has no default shortcut; assign one in **Settings → Keybindings**. -`thread.undo` (`mod+z` by default) reverses the most recent thread action that is -still offering **Undo** in a notification, such as an unpin, settle, snooze, or -archive. Its default rule skips text fields and terminals so native undo keeps -working there. +`thread.undo` (`mod+z` by default) reverses the actions shown in the notice at the +bottom of the sidebar, such as unpin, settle, snooze, or archive. Consecutive +actions of the same kind undo together. The notice remains available for five +seconds after the latest action. The default shortcut skips text fields and +terminals so native undo keeps working there. `chat.new` may ask you to choose a project when there is more than one. `chat.newLocal` skips that chooser. Both use your From 5423ba0fd08447240282c08362ee8e382bf16903 Mon Sep 17 00:00:00 2001 From: Bilal Bakr <62337003+Bil0000@users.noreply.github.com> Date: Tue, 22 Sep 2026 05:51:35 +0300 Subject: [PATCH 2/7] feat(web): merge the comment and review buttons into one composer (#12945) Co-authored-by: maria-rcks --- .../pullRequest/PullRequestCodeTab.tsx | 90 +------- .../PullRequestCommentComposer.tsx | 171 --------------- .../pullRequest/PullRequestCommentForm.tsx | 145 +++++++++++++ .../pullRequest/PullRequestComposer.tsx | 195 ++++++++++++++++++ .../pullRequest/PullRequestDetailPanel.tsx | 7 +- ...eviewBar.tsx => PullRequestReviewForm.tsx} | 95 +++++---- apps/web/src/components/ui/popover.tsx | 11 +- 7 files changed, 413 insertions(+), 301 deletions(-) delete mode 100644 apps/web/src/components/pullRequest/PullRequestCommentComposer.tsx create mode 100644 apps/web/src/components/pullRequest/PullRequestCommentForm.tsx create mode 100644 apps/web/src/components/pullRequest/PullRequestComposer.tsx rename apps/web/src/components/pullRequest/{PullRequestReviewBar.tsx => PullRequestReviewForm.tsx} (64%) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 08a46872b69..131593ab6d2 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -18,13 +18,11 @@ import { Columns2Icon, FolderTreeIcon, InfoIcon, - MessageSquareIcon, MessageSquareOffIcon, PilcrowIcon, Rows3Icon, TextWrapIcon, TriangleAlertIcon, - XIcon, } from "lucide-react"; import { useAtomRefresh } from "@effect/atom-react"; import * as Schema from "effect/Schema"; @@ -78,7 +76,6 @@ import { toastManager } from "../ui/toast"; import { Toggle, ToggleGroup } from "../ui/toggle-group"; import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { PendingReviewCommentCard, ReviewThreadCard } from "./PullRequestReviewAnnotation"; -import { PullRequestReviewBar } from "./PullRequestReviewBar"; import { isFileDiffCollapsed, isLineInFileDiff, @@ -247,9 +244,6 @@ function PullRequestCodeTab({ const [draft, setDraft] = useState(null); const [threadPending, setThreadPending] = useState(false); const [orphansOpen, setOrphansOpen] = useState(false); - // Closed by default so the review form does not permanently eat vertical space below the - // diff; opened on demand as a floating overlay instead. - const [reviewOpen, setReviewOpen] = useState(false); // Which pull request the slices belong to travels with them, so a render taken before the // reset below cannot read the previous one's slices — or send its cursor to the host. const [sliceState, setSliceState] = useState<{ @@ -378,7 +372,6 @@ function PullRequestCodeTab({ inlineComment: hostReview.inlineComment && viewer.comment, reply: hostReview.reply && viewer.comment, resolve: hostReview.resolve && viewer.resolve, - verdicts: hostReview.verdicts.filter((verdict) => viewer.verdicts.includes(verdict)), }; }, [detail.capabilities.review, detail.viewerPermissions]); // A comment is posted against the pull request's head diff, so a line number taken from one @@ -1054,70 +1047,6 @@ function PullRequestCodeTab({ ], ); - /** - * The review overlay belongs to the pull request, not to the patch: a change whose diff - * cannot be structured — or read at all — is still one a reviewer can approve or reject, so - * it survives every branch below. It floats over the scroll area rather than sitting in the - * layout flow, so the diff keeps the full height instead of permanently losing a strip to a - * footer most reviews never touch. Hidden entirely where the host offers no verdicts, same as - * the bar it wraps did. - */ - const reviewOverlay = - review.verdicts.length === 0 ? null : ( -
- {reviewOpen ? ( -
- - { - onRefresh(); - setReviewOpen(false); - }} - /> -
- ) : ( - - )} -
- ); // A rebase or a force-push can take the scoped commit out of the change. Its diff may still // be reachable on the host, but it is no longer part of what is being reviewed, so the scope // goes back to the whole change rather than sitting under a name nothing matches. @@ -1392,29 +1321,23 @@ function PullRequestCodeTab({ ); // The toolbar rides above every branch below, not just the one with a patch in it: a commit // whose diff is empty or unreadable still needs the scope dropdown that got the reader there. - const withReviewBar = (body: ReactNode) => ( + const withToolbar = (body: ReactNode) => (
{toolbar} - {/* The overlay is anchored to this wrapper, not the scroller: absolute positioning - inside an overflowing element tracks the content's bottom edge, which would carry - the trigger away with the first scroll. */} -
-
{body}
- {reviewOverlay} -
+
{body}
); // Under the toolbar rather than in place of it, so choosing a commit does not take the // dropdown that was just used off the screen while its diff loads. if (diffQuery.isPending && loadedSlices.length === 0) { - return withReviewBar(); + return withToolbar(); } // A slice that fails once there are files on screen is reported at the end of them instead: // the diff already read is worth more than the error that stopped it growing. if (diffQuery.error && loadedSlices.length === 0) { - return withReviewBar( + return withToolbar(

{diffQuery.error}

, ); } @@ -1428,7 +1351,7 @@ function PullRequestCodeTab({ ? parsedSlices.flatMap((parsed) => (parsed?.kind === "raw" ? [parsed] : [])) : []; if (files.length === 0 && rawSlices.length > 0) { - return withReviewBar( + return withToolbar(
{rawSlices.map((slice) => (
@@ -1441,7 +1364,7 @@ function PullRequestCodeTab({ } if (items.length === 0 && nextCursor === null) { - return withReviewBar( + return withToolbar(

{commit === null ? "This pull request has no file changes." @@ -1595,7 +1518,6 @@ function PullRequestCodeTab({ renderAnnotation={renderAnnotation} unsafeCSSExtra={REPLACE_FILE_COUNTS_CSS} /> - {reviewOverlay}

{fileTreeOpen ? (