fix(webapp): keep the delete-chat confirmation alive outside the history popover
The confirmation dialog was rendered inside the non-modal history popover, so taking focus dismissed the popover and unmounted the dialog with it. It now lives in the header as a sibling of the popover, and the panel's Escape handler only fires for events whose target is inside the panel.
This commit is contained in:
committed by
Katia Bulatova
parent
ed67d38c02
commit
adae596279
@@ -6,7 +6,11 @@ import { Button } from "~/components/primitives/Buttons";
|
||||
import { Popover, PopoverArrowTrigger, PopoverContent } from "~/components/primitives/Popover";
|
||||
import { ShortcutKey } from "~/components/primitives/ShortcutKey";
|
||||
import type { Shortcut } from "~/hooks/useShortcutKeys";
|
||||
import { DashboardAgentHistoryMenu, type DashboardAgentChat } from "./DashboardAgentHistory";
|
||||
import {
|
||||
DashboardAgentDeleteChatDialog,
|
||||
DashboardAgentHistoryMenu,
|
||||
type DashboardAgentChat,
|
||||
} from "./DashboardAgentHistory";
|
||||
import { chatHistoryTriggerLabel } from "./header-labels";
|
||||
|
||||
// Display only. The key is registered once, in `DashboardAgent`; registering it
|
||||
@@ -45,6 +49,7 @@ export function DashboardAgentHeader({
|
||||
onClose: () => void;
|
||||
}) {
|
||||
const [isHistoryOpen, setHistoryOpen] = useState(false);
|
||||
const [pendingDelete, setPendingDelete] = useState<DashboardAgentChat | null>(null);
|
||||
|
||||
return (
|
||||
<div className="flex h-10 shrink-0 items-center justify-between gap-2 border-b border-grid-bright pl-1 pr-1.5">
|
||||
@@ -77,11 +82,20 @@ export function DashboardAgentHeader({
|
||||
setHistoryOpen(false);
|
||||
onSelectChat(chatId);
|
||||
}}
|
||||
onDelete={onDeleteChat}
|
||||
onRequestDelete={(chat) => {
|
||||
setHistoryOpen(false);
|
||||
setPendingDelete(chat);
|
||||
}}
|
||||
/>
|
||||
</PopoverContent>
|
||||
</Popover>
|
||||
|
||||
<DashboardAgentDeleteChatDialog
|
||||
chat={pendingDelete}
|
||||
onOpenChange={(open) => !open && setPendingDelete(null)}
|
||||
onConfirm={onDeleteChat}
|
||||
/>
|
||||
|
||||
<div className="flex shrink-0 items-center gap-0.5">
|
||||
{showNewChat && (
|
||||
<Button
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
import { MagnifyingGlassIcon, TrashIcon } from "@heroicons/react/20/solid";
|
||||
import { formatDurationMilliseconds } from "@trigger.dev/core/v3/utils/durations";
|
||||
import { useState } from "react";
|
||||
import { Button } from "~/components/primitives/Buttons";
|
||||
import { Dialog, DialogContent, DialogHeader } from "~/components/primitives/Dialog";
|
||||
import { FormButtons } from "~/components/primitives/FormButtons";
|
||||
@@ -82,87 +81,95 @@ export function DashboardAgentHistoryMenu({
|
||||
currentChatId,
|
||||
thinkingChatId,
|
||||
onSelect,
|
||||
onDelete,
|
||||
onRequestDelete,
|
||||
}: {
|
||||
chats: DashboardAgentChat[];
|
||||
currentChatId: string;
|
||||
thinkingChatId?: string | null;
|
||||
onSelect: (chatId: string) => void;
|
||||
onDelete: (chatId: string) => void;
|
||||
onRequestDelete: (chat: DashboardAgentChat) => void;
|
||||
}) {
|
||||
const [pendingDelete, setPendingDelete] = useState<DashboardAgentChat | null>(null);
|
||||
const now = Date.now();
|
||||
|
||||
return (
|
||||
<>
|
||||
<div className="max-h-80 overflow-y-auto p-1.5 scrollbar-thin scrollbar-track-transparent scrollbar-thumb-surface-control">
|
||||
{chats.length === 0 ? (
|
||||
<Paragraph variant="small" className="p-1.5 text-text-dimmed">
|
||||
No previous chats yet.
|
||||
</Paragraph>
|
||||
) : (
|
||||
<AgentList>
|
||||
{unreadFirst(chats).map((chat) => {
|
||||
const process = chatProcess(chat, chat.id === thinkingChatId);
|
||||
const age = chat.lastMessageAt ? chatAge(chat.lastMessageAt, now) : undefined;
|
||||
return (
|
||||
<AgentListRow
|
||||
key={chat.id}
|
||||
label={chat.title}
|
||||
unread={chatIsUnread(chat)}
|
||||
status={process ? <ProcessIcon process={process} /> : null}
|
||||
meta={age}
|
||||
variant={chat.id === currentChatId ? "selected" : "default"}
|
||||
onSelect={() => onSelect(chat.id)}
|
||||
action={
|
||||
<AgentListRowAction
|
||||
icon={TrashIcon}
|
||||
label={`Delete chat: ${chat.title}`}
|
||||
onClick={() => setPendingDelete(chat)}
|
||||
danger
|
||||
/>
|
||||
}
|
||||
/>
|
||||
);
|
||||
})}
|
||||
</AgentList>
|
||||
)}
|
||||
</div>
|
||||
|
||||
<Dialog
|
||||
open={pendingDelete !== null}
|
||||
onOpenChange={(open) => !open && setPendingDelete(null)}
|
||||
>
|
||||
<DialogContent>
|
||||
<DialogHeader>Delete this chat?</DialogHeader>
|
||||
<div className="flex flex-col gap-3 pt-3">
|
||||
<Paragraph>
|
||||
"{pendingDelete?.title}" and everything in it will be deleted. This can't be undone.
|
||||
</Paragraph>
|
||||
<FormButtons
|
||||
confirmButton={
|
||||
<Button
|
||||
type="button"
|
||||
variant="danger/medium"
|
||||
LeadingIcon={TrashIcon}
|
||||
shortcut={{ modifiers: ["mod"], key: "enter" }}
|
||||
onClick={() => {
|
||||
if (pendingDelete) onDelete(pendingDelete.id);
|
||||
setPendingDelete(null);
|
||||
}}
|
||||
>
|
||||
Delete chat
|
||||
</Button>
|
||||
}
|
||||
cancelButton={
|
||||
<Button variant="tertiary/medium" onClick={() => setPendingDelete(null)}>
|
||||
Cancel
|
||||
</Button>
|
||||
}
|
||||
/>
|
||||
</div>
|
||||
</DialogContent>
|
||||
</Dialog>
|
||||
</>
|
||||
<div className="max-h-80 overflow-y-auto p-1.5 scrollbar-thin scrollbar-track-transparent scrollbar-thumb-surface-control">
|
||||
{chats.length === 0 ? (
|
||||
<Paragraph variant="small" className="p-1.5 text-text-dimmed">
|
||||
No previous chats yet.
|
||||
</Paragraph>
|
||||
) : (
|
||||
<AgentList>
|
||||
{unreadFirst(chats).map((chat) => {
|
||||
const process = chatProcess(chat, chat.id === thinkingChatId);
|
||||
const age = chat.lastMessageAt ? chatAge(chat.lastMessageAt, now) : undefined;
|
||||
return (
|
||||
<AgentListRow
|
||||
key={chat.id}
|
||||
label={chat.title}
|
||||
unread={chatIsUnread(chat)}
|
||||
status={process ? <ProcessIcon process={process} /> : null}
|
||||
meta={age}
|
||||
variant={chat.id === currentChatId ? "selected" : "default"}
|
||||
onSelect={() => onSelect(chat.id)}
|
||||
action={
|
||||
<AgentListRowAction
|
||||
icon={TrashIcon}
|
||||
label={`Delete chat: ${chat.title}`}
|
||||
onClick={() => onRequestDelete(chat)}
|
||||
danger
|
||||
/>
|
||||
}
|
||||
/>
|
||||
);
|
||||
})}
|
||||
</AgentList>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
// Rendered outside the history popover: inside it, focus moving to the dialog dismisses the
|
||||
// popover, which unmounts the dialog before it can be answered.
|
||||
export function DashboardAgentDeleteChatDialog({
|
||||
chat,
|
||||
onOpenChange,
|
||||
onConfirm,
|
||||
}: {
|
||||
chat: DashboardAgentChat | null;
|
||||
onOpenChange: (open: boolean) => void;
|
||||
onConfirm: (chatId: string) => void;
|
||||
}) {
|
||||
return (
|
||||
<Dialog open={chat !== null} onOpenChange={onOpenChange}>
|
||||
<DialogContent>
|
||||
<DialogHeader>Delete this chat?</DialogHeader>
|
||||
<div className="flex flex-col gap-3 pt-3">
|
||||
<Paragraph>
|
||||
"{chat?.title}" and everything in it will be deleted. This can't be undone.
|
||||
</Paragraph>
|
||||
<FormButtons
|
||||
confirmButton={
|
||||
<Button
|
||||
type="button"
|
||||
variant="danger/medium"
|
||||
LeadingIcon={TrashIcon}
|
||||
shortcut={{ modifiers: ["mod"], key: "enter" }}
|
||||
onClick={() => {
|
||||
if (chat) onConfirm(chat.id);
|
||||
onOpenChange(false);
|
||||
}}
|
||||
>
|
||||
Delete chat
|
||||
</Button>
|
||||
}
|
||||
cancelButton={
|
||||
<Button variant="tertiary/medium" onClick={() => onOpenChange(false)}>
|
||||
Cancel
|
||||
</Button>
|
||||
}
|
||||
/>
|
||||
</div>
|
||||
</DialogContent>
|
||||
</Dialog>
|
||||
);
|
||||
}
|
||||
|
||||
@@ -525,10 +525,18 @@ export function DashboardAgentPanel({
|
||||
|
||||
return (
|
||||
<div
|
||||
ref={panelRef}
|
||||
className="flex h-full flex-col bg-background-bright animate-in slide-in-from-right-2 duration-150"
|
||||
// A React handler, not a global hotkey, so Esc stays scoped to the panel.
|
||||
onKeyDown={(event) => {
|
||||
if (event.key !== "Escape" || event.defaultPrevented) return;
|
||||
if (
|
||||
!escapeClosesPanel({
|
||||
key: event.key,
|
||||
defaultPrevented: event.defaultPrevented,
|
||||
targetInsidePanel: panelRef.current?.contains(event.target as Node) ?? false,
|
||||
})
|
||||
)
|
||||
return;
|
||||
event.preventDefault();
|
||||
onClose();
|
||||
}}
|
||||
|
||||
@@ -0,0 +1,75 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { escapeClosesPanel } from "./panel-escape";
|
||||
|
||||
/**
|
||||
* Escape has to reach the thing the user meant. Radix dismisses a popover or a dialog from a
|
||||
* document listener that runs after the panel's own handler and never marks the event handled,
|
||||
* so the panel has to decide for itself whether the keystroke came from inside it.
|
||||
*/
|
||||
describe("escapeClosesPanel", () => {
|
||||
it("closes the panel when Escape comes from the panel itself", () => {
|
||||
expect(
|
||||
escapeClosesPanel({ key: "Escape", defaultPrevented: false, targetInsidePanel: true })
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("leaves the panel open when Escape comes from a portalled layer", () => {
|
||||
// The history popover and the delete dialog both render outside the panel's DOM subtree.
|
||||
expect(
|
||||
escapeClosesPanel({ key: "Escape", defaultPrevented: false, targetInsidePanel: false })
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("stays out of the way once something else has handled the key", () => {
|
||||
expect(
|
||||
escapeClosesPanel({ key: "Escape", defaultPrevented: true, targetInsidePanel: true })
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it("ignores every other key", () => {
|
||||
expect(
|
||||
escapeClosesPanel({ key: "Enter", defaultPrevented: false, targetInsidePanel: true })
|
||||
).toBe(false);
|
||||
expect(escapeClosesPanel({ key: "j", defaultPrevented: false, targetInsidePanel: true })).toBe(
|
||||
false
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Structural guards, not behavioural proof: the delete confirmation's survival depends on where
|
||||
* it is mounted in the tree, which these assertions pin down without rendering anything.
|
||||
*/
|
||||
describe("the delete confirmation lives outside the history popover", () => {
|
||||
const header = readFileSync(new URL("./DashboardAgentHeader.tsx", import.meta.url), "utf8");
|
||||
const history = readFileSync(new URL("./DashboardAgentHistory.tsx", import.meta.url), "utf8");
|
||||
const panel = readFileSync(new URL("./DashboardAgentPanel.tsx", import.meta.url), "utf8");
|
||||
|
||||
const menuBody = history.slice(
|
||||
history.indexOf("export function DashboardAgentHistoryMenu"),
|
||||
history.indexOf("export function DashboardAgentDeleteChatDialog")
|
||||
);
|
||||
|
||||
it("keeps no dialog and no pending state inside the popover's menu", () => {
|
||||
expect(menuBody).not.toContain("<Dialog");
|
||||
expect(menuBody).not.toContain("useState");
|
||||
});
|
||||
|
||||
it("mounts the dialog in the header as a sibling of the popover, not within it", () => {
|
||||
const popoverEnd = header.indexOf("</Popover>");
|
||||
const dialog = header.indexOf("<DashboardAgentDeleteChatDialog");
|
||||
expect(popoverEnd).toBeGreaterThan(-1);
|
||||
expect(dialog).toBeGreaterThan(popoverEnd);
|
||||
});
|
||||
|
||||
it("owns the pending chat in the header, so dismissing the popover cannot unmount it", () => {
|
||||
expect(header).toContain("const [pendingDelete, setPendingDelete] = useState");
|
||||
});
|
||||
|
||||
it("gates the panel's Escape on the shared rule rather than defaultPrevented alone", () => {
|
||||
expect(panel).toContain("escapeClosesPanel({");
|
||||
expect(panel).toContain("panelRef.current?.contains(event.target as Node)");
|
||||
expect(panel).not.toContain('if (event.key !== "Escape" || event.defaultPrevented) return;');
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,15 @@
|
||||
/**
|
||||
* Escape inside the panel closes the panel — but a popover or a dialog is portalled out of
|
||||
* the panel's DOM subtree while still bubbling through the React tree, and Radix dismisses
|
||||
* those from a document listener that runs after this handler, so the event arrives here
|
||||
* undefaulted. Deciding on the DOM target is what tells the two apart.
|
||||
*/
|
||||
export function escapeClosesPanel(event: {
|
||||
key: string;
|
||||
defaultPrevented: boolean;
|
||||
/** Whether the event's target is a DOM descendant of the panel. */
|
||||
targetInsidePanel: boolean;
|
||||
}): boolean {
|
||||
if (event.key !== "Escape" || event.defaultPrevented) return false;
|
||||
return event.targetInsidePanel;
|
||||
}
|
||||
Reference in New Issue
Block a user