Skip to content

Commit 712396b

Browse files
kathiekiwikathiekiwi
committed
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.
1 parent f059df6 commit 712396b

5 files changed

Lines changed: 195 additions & 76 deletions

File tree

‎apps/webapp/app/components/dashboard-agent/DashboardAgentHeader.tsx‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ import { Button } from "~/components/primitives/Buttons";
66
import { Popover, PopoverArrowTrigger, PopoverContent } from "~/components/primitives/Popover";
77
import { ShortcutKey } from "~/components/primitives/ShortcutKey";
88
import type { Shortcut } from "~/hooks/useShortcutKeys";
9-
import { DashboardAgentHistoryMenu, type DashboardAgentChat } from "./DashboardAgentHistory";
9+
import {
10+
DashboardAgentDeleteChatDialog,
11+
DashboardAgentHistoryMenu,
12+
type DashboardAgentChat,
13+
} from "./DashboardAgentHistory";
1014

1115
// Display only. The key is registered once, in `DashboardAgent`; registering it
1216
// anywhere else makes the keystroke fire twice.
@@ -44,6 +48,7 @@ export function DashboardAgentHeader({
4448
onClose: () => void;
4549
}) {
4650
const [isHistoryOpen, setHistoryOpen] = useState(false);
51+
const [pendingDelete, setPendingDelete] = useState<DashboardAgentChat | null>(null);
4752

4853
return (
4954
<div className="flex h-10 shrink-0 items-center justify-between gap-2 border-b border-grid-bright pl-1 pr-1.5">
@@ -76,11 +81,20 @@ export function DashboardAgentHeader({
7681
setHistoryOpen(false);
7782
onSelectChat(chatId);
7883
}}
79-
onDelete={onDeleteChat}
84+
onRequestDelete={(chat) => {
85+
setHistoryOpen(false);
86+
setPendingDelete(chat);
87+
}}
8088
/>
8189
</PopoverContent>
8290
</Popover>
8391

92+
<DashboardAgentDeleteChatDialog
93+
chat={pendingDelete}
94+
onOpenChange={(open) => !open && setPendingDelete(null)}
95+
onConfirm={onDeleteChat}
96+
/>
97+
8498
<div className="flex shrink-0 items-center gap-0.5">
8599
{showNewChat && (
86100
<Button

‎apps/webapp/app/components/dashboard-agent/DashboardAgentHistory.tsx‎

Lines changed: 80 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { MagnifyingGlassIcon, TrashIcon } from "@heroicons/react/20/solid";
22
import { formatDurationMilliseconds } from "@trigger.dev/core/v3/utils/durations";
3-
import { useState } from "react";
43
import { Button } from "~/components/primitives/Buttons";
54
import { Dialog, DialogContent, DialogHeader } from "~/components/primitives/Dialog";
65
import { FormButtons } from "~/components/primitives/FormButtons";
@@ -82,87 +81,95 @@ export function DashboardAgentHistoryMenu({
8281
currentChatId,
8382
thinkingChatId,
8483
onSelect,
85-
onDelete,
84+
onRequestDelete,
8685
}: {
8786
chats: DashboardAgentChat[];
8887
currentChatId: string;
8988
thinkingChatId?: string | null;
9089
onSelect: (chatId: string) => void;
91-
onDelete: (chatId: string) => void;
90+
onRequestDelete: (chat: DashboardAgentChat) => void;
9291
}) {
93-
const [pendingDelete, setPendingDelete] = useState<DashboardAgentChat | null>(null);
9492
const now = Date.now();
9593

9694
return (
97-
<>
98-
<div className="max-h-80 overflow-y-auto p-1.5 scrollbar-thin scrollbar-track-transparent scrollbar-thumb-surface-control">
99-
{chats.length === 0 ? (
100-
<Paragraph variant="small" className="p-1.5 text-text-dimmed">
101-
No previous chats yet.
102-
</Paragraph>
103-
) : (
104-
<AgentList>
105-
{unreadFirst(chats).map((chat) => {
106-
const process = chatProcess(chat, chat.id === thinkingChatId);
107-
const age = chat.lastMessageAt ? chatAge(chat.lastMessageAt, now) : undefined;
108-
return (
109-
<AgentListRow
110-
key={chat.id}
111-
label={chat.title}
112-
unread={chatIsUnread(chat)}
113-
status={process ? <ProcessIcon process={process} /> : null}
114-
meta={age}
115-
variant={chat.id === currentChatId ? "selected" : "default"}
116-
onSelect={() => onSelect(chat.id)}
117-
action={
118-
<AgentListRowAction
119-
icon={TrashIcon}
120-
label={`Delete chat: ${chat.title}`}
121-
onClick={() => setPendingDelete(chat)}
122-
danger
123-
/>
124-
}
125-
/>
126-
);
127-
})}
128-
</AgentList>
129-
)}
130-
</div>
95+
<div className="max-h-80 overflow-y-auto p-1.5 scrollbar-thin scrollbar-track-transparent scrollbar-thumb-surface-control">
96+
{chats.length === 0 ? (
97+
<Paragraph variant="small" className="p-1.5 text-text-dimmed">
98+
No previous chats yet.
99+
</Paragraph>
100+
) : (
101+
<AgentList>
102+
{unreadFirst(chats).map((chat) => {
103+
const process = chatProcess(chat, chat.id === thinkingChatId);
104+
const age = chat.lastMessageAt ? chatAge(chat.lastMessageAt, now) : undefined;
105+
return (
106+
<AgentListRow
107+
key={chat.id}
108+
label={chat.title}
109+
unread={chatIsUnread(chat)}
110+
status={process ? <ProcessIcon process={process} /> : null}
111+
meta={age}
112+
variant={chat.id === currentChatId ? "selected" : "default"}
113+
onSelect={() => onSelect(chat.id)}
114+
action={
115+
<AgentListRowAction
116+
icon={TrashIcon}
117+
label={`Delete chat: ${chat.title}`}
118+
onClick={() => onRequestDelete(chat)}
119+
danger
120+
/>
121+
}
122+
/>
123+
);
124+
})}
125+
</AgentList>
126+
)}
127+
</div>
128+
);
129+
}
131130

132-
<Dialog
133-
open={pendingDelete !== null}
134-
onOpenChange={(open) => !open && setPendingDelete(null)}
135-
>
136-
<DialogContent>
137-
<DialogHeader>Delete this chat?</DialogHeader>
138-
<div className="flex flex-col gap-3 pt-3">
139-
<Paragraph>
140-
"{pendingDelete?.title}" and everything in it will be deleted. This can't be undone.
141-
</Paragraph>
142-
<FormButtons
143-
confirmButton={
144-
<Button
145-
type="button"
146-
variant="danger/medium"
147-
LeadingIcon={TrashIcon}
148-
shortcut={{ modifiers: ["mod"], key: "enter" }}
149-
onClick={() => {
150-
if (pendingDelete) onDelete(pendingDelete.id);
151-
setPendingDelete(null);
152-
}}
153-
>
154-
Delete chat
155-
</Button>
156-
}
157-
cancelButton={
158-
<Button variant="tertiary/medium" onClick={() => setPendingDelete(null)}>
159-
Cancel
160-
</Button>
161-
}
162-
/>
163-
</div>
164-
</DialogContent>
165-
</Dialog>
166-
</>
131+
// Rendered outside the history popover: inside it, focus moving to the dialog dismisses the
132+
// popover, which unmounts the dialog before it can be answered.
133+
export function DashboardAgentDeleteChatDialog({
134+
chat,
135+
onOpenChange,
136+
onConfirm,
137+
}: {
138+
chat: DashboardAgentChat | null;
139+
onOpenChange: (open: boolean) => void;
140+
onConfirm: (chatId: string) => void;
141+
}) {
142+
return (
143+
<Dialog open={chat !== null} onOpenChange={onOpenChange}>
144+
<DialogContent>
145+
<DialogHeader>Delete this chat?</DialogHeader>
146+
<div className="flex flex-col gap-3 pt-3">
147+
<Paragraph>
148+
"{chat?.title}" and everything in it will be deleted. This can't be undone.
149+
</Paragraph>
150+
<FormButtons
151+
confirmButton={
152+
<Button
153+
type="button"
154+
variant="danger/medium"
155+
LeadingIcon={TrashIcon}
156+
shortcut={{ modifiers: ["mod"], key: "enter" }}
157+
onClick={() => {
158+
if (chat) onConfirm(chat.id);
159+
onOpenChange(false);
160+
}}
161+
>
162+
Delete chat
163+
</Button>
164+
}
165+
cancelButton={
166+
<Button variant="tertiary/medium" onClick={() => onOpenChange(false)}>
167+
Cancel
168+
</Button>
169+
}
170+
/>
171+
</div>
172+
</DialogContent>
173+
</Dialog>
167174
);
168175
}

‎apps/webapp/app/components/dashboard-agent/DashboardAgentPanel.tsx‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,10 +545,18 @@ export function DashboardAgentPanel({
545545

546546
return (
547547
<div
548+
ref={panelRef}
548549
className="flex h-full flex-col bg-background-bright animate-in slide-in-from-right-2 duration-150"
549550
// A React handler, not a global hotkey, so Esc stays scoped to the panel.
550551
onKeyDown={(event) => {
551-
if (event.key !== "Escape" || event.defaultPrevented) return;
552+
if (
553+
!escapeClosesPanel({
554+
key: event.key,
555+
defaultPrevented: event.defaultPrevented,
556+
targetInsidePanel: panelRef.current?.contains(event.target as Node) ?? false,
557+
})
558+
)
559+
return;
552560
event.preventDefault();
553561
onClose();
554562
}}

‎apps/webapp/app/components/dashboard-agent/panel-escape.test.ts‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { readFileSync } from "node:fs";
2+
import { describe, expect, it } from "vitest";
3+
import { escapeClosesPanel } from "./panel-escape";
4+
5+
/**
6+
* Escape has to reach the thing the user meant. Radix dismisses a popover or a dialog from a
7+
* document listener that runs after the panel's own handler and never marks the event handled,
8+
* so the panel has to decide for itself whether the keystroke came from inside it.
9+
*/
10+
describe("escapeClosesPanel", () => {
11+
it("closes the panel when Escape comes from the panel itself", () => {
12+
expect(
13+
escapeClosesPanel({ key: "Escape", defaultPrevented: false, targetInsidePanel: true })
14+
).toBe(true);
15+
});
16+
17+
it("leaves the panel open when Escape comes from a portalled layer", () => {
18+
// The history popover and the delete dialog both render outside the panel's DOM subtree.
19+
expect(
20+
escapeClosesPanel({ key: "Escape", defaultPrevented: false, targetInsidePanel: false })
21+
).toBe(false);
22+
});
23+
24+
it("stays out of the way once something else has handled the key", () => {
25+
expect(
26+
escapeClosesPanel({ key: "Escape", defaultPrevented: true, targetInsidePanel: true })
27+
).toBe(false);
28+
});
29+
30+
it("ignores every other key", () => {
31+
expect(
32+
escapeClosesPanel({ key: "Enter", defaultPrevented: false, targetInsidePanel: true })
33+
).toBe(false);
34+
expect(escapeClosesPanel({ key: "j", defaultPrevented: false, targetInsidePanel: true })).toBe(
35+
false
36+
);
37+
});
38+
});
39+
40+
/**
41+
* Structural guards, not behavioural proof: the delete confirmation's survival depends on where
42+
* it is mounted in the tree, which these assertions pin down without rendering anything.
43+
*/
44+
describe("the delete confirmation lives outside the history popover", () => {
45+
const header = readFileSync(new URL("./DashboardAgentHeader.tsx", import.meta.url), "utf8");
46+
const history = readFileSync(new URL("./DashboardAgentHistory.tsx", import.meta.url), "utf8");
47+
const panel = readFileSync(new URL("./DashboardAgentPanel.tsx", import.meta.url), "utf8");
48+
49+
const menuBody = history.slice(
50+
history.indexOf("export function DashboardAgentHistoryMenu"),
51+
history.indexOf("export function DashboardAgentDeleteChatDialog")
52+
);
53+
54+
it("keeps no dialog and no pending state inside the popover's menu", () => {
55+
expect(menuBody).not.toContain("<Dialog");
56+
expect(menuBody).not.toContain("useState");
57+
});
58+
59+
it("mounts the dialog in the header as a sibling of the popover, not within it", () => {
60+
const popoverEnd = header.indexOf("</Popover>");
61+
const dialog = header.indexOf("<DashboardAgentDeleteChatDialog");
62+
expect(popoverEnd).toBeGreaterThan(-1);
63+
expect(dialog).toBeGreaterThan(popoverEnd);
64+
});
65+
66+
it("owns the pending chat in the header, so dismissing the popover cannot unmount it", () => {
67+
expect(header).toContain("const [pendingDelete, setPendingDelete] = useState");
68+
});
69+
70+
it("gates the panel's Escape on the shared rule rather than defaultPrevented alone", () => {
71+
expect(panel).toContain("escapeClosesPanel({");
72+
expect(panel).toContain("panelRef.current?.contains(event.target as Node)");
73+
expect(panel).not.toContain('if (event.key !== "Escape" || event.defaultPrevented) return;');
74+
});
75+
});

‎apps/webapp/app/components/dashboard-agent/panel-escape.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* Escape inside the panel closes the panel — but a popover or a dialog is portalled out of
3+
* the panel's DOM subtree while still bubbling through the React tree, and Radix dismisses
4+
* those from a document listener that runs after this handler, so the event arrives here
5+
* undefaulted. Deciding on the DOM target is what tells the two apart.
6+
*/
7+
export function escapeClosesPanel(event: {
8+
key: string;
9+
defaultPrevented: boolean;
10+
/** Whether the event's target is a DOM descendant of the panel. */
11+
targetInsidePanel: boolean;
12+
}): boolean {
13+
if (event.key !== "Escape" || event.defaultPrevented) return false;
14+
return event.targetInsidePanel;
15+
}

0 commit comments

Comments
 (0)