From 8faefb88be292852187da4068decebccf2d384d0 Mon Sep 17 00:00:00 2001 From: Matthew Meszaros Date: Thu, 1 Oct 2026 10:14:55 -0700 Subject: [PATCH] feat: leave Escape to a confirm or modal opened above an open dropdown in useClickOutside, drop ImageMenu's duplicate Escape guard, and restore the useDismissAdvisorFinding and useDismissMailboxImport mutation names the hook rename caught --- .../components/app/advisor/AdvisorCard.tsx | 4 ++-- .../app/campaigns/sequences/ImageControls.tsx | 14 ------------ .../app/emails/import/MailboxImportsMenu.tsx | 4 ++-- web/src/hooks/useClickOutside.test.tsx | 22 +++++++++++++++++++ web/src/hooks/useClickOutside.ts | 4 ++++ .../lib/api/hooks/app/advisor/useAdvisor.ts | 2 +- .../app/emails/useMailboxImportActions.ts | 2 +- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/web/src/components/app/advisor/AdvisorCard.tsx b/web/src/components/app/advisor/AdvisorCard.tsx index 69790e2c8..6e59a6547 100644 --- a/web/src/components/app/advisor/AdvisorCard.tsx +++ b/web/src/components/app/advisor/AdvisorCard.tsx @@ -39,7 +39,7 @@ import { import { useConfirm } from "@/hooks/context/confirm"; import { useAdvisorFeedback, - useClickOutsideAdvisorFinding, + useDismissAdvisorFinding, useSnoozeAdvisorFinding, useUndoAdvisorFinding, } from "@/lib/api/hooks/app/advisor/useAdvisor"; @@ -63,7 +63,7 @@ export default function AdvisorCard({ finding, onFix, compact = false, defaultOp const confirm = useConfirm(); const snooze = useSnoozeAdvisorFinding(); - const dismiss = useClickOutsideAdvisorFinding(); + const dismiss = useDismissAdvisorFinding(); const undo = useUndoAdvisorFinding(); const feedback = useAdvisorFeedback(); diff --git a/web/src/components/app/campaigns/sequences/ImageControls.tsx b/web/src/components/app/campaigns/sequences/ImageControls.tsx index aaefc5625..05af1845e 100644 --- a/web/src/components/app/campaigns/sequences/ImageControls.tsx +++ b/web/src/components/app/campaigns/sequences/ImageControls.tsx @@ -59,20 +59,6 @@ export function ImageMenu({ editor }: { editor: Editor }) { const del = useDeleteEmailImage(); const confirm = useConfirm(); - React.useEffect(() => { - if (!open) return; - const onKey = (e: KeyboardEvent) => { - if (e.key !== "Escape") return; - // The delete confirmation sits above this menu, so Escape belongs - // to it first: only the innermost layer closes. - if (document.querySelector("[role='alertdialog']")) return; - e.stopPropagation(); - setOpen(false); - }; - document.addEventListener("keydown", onKey, true); - return () => document.removeEventListener("keydown", onKey, true); - }, [open]); - const pick = async (files: FileList | File[] | null) => { const file = Array.from(files ?? [])[0]; if (!file) return; diff --git a/web/src/components/app/emails/import/MailboxImportsMenu.tsx b/web/src/components/app/emails/import/MailboxImportsMenu.tsx index b519fb9f4..2de1c7528 100644 --- a/web/src/components/app/emails/import/MailboxImportsMenu.tsx +++ b/web/src/components/app/emails/import/MailboxImportsMenu.tsx @@ -11,7 +11,7 @@ import { PopoverMenuTrigger, } from "@/components/ui/popover-menu"; import useMailboxImports from "@/lib/api/hooks/app/emails/useMailboxImports"; -import { useClickOutsideMailboxImport } from "@/lib/api/hooks/app/emails/useMailboxImportActions"; +import { useDismissMailboxImport } from "@/lib/api/hooks/app/emails/useMailboxImportActions"; import { type MailboxImport } from "@/lib/api/models/app/emails/MailboxImport"; import { vendorLabel } from "@/lib/api/models/app/emails/MailboxSources"; import { useConfirm } from "@/hooks/context/confirm"; @@ -143,7 +143,7 @@ export default function MailboxImportsMenu() { function ImportEntry({ job, onOpen }: { job: MailboxImport; onOpen: () => void }) { const confirm = useConfirm(); - const dismiss = useClickOutsideMailboxImport(); + const dismiss = useDismissMailboxImport(); const st = jobState(job); const live = job.status === "running"; diff --git a/web/src/hooks/useClickOutside.test.tsx b/web/src/hooks/useClickOutside.test.tsx index e8eaee8d5..e2ede53f2 100644 --- a/web/src/hooks/useClickOutside.test.tsx +++ b/web/src/hooks/useClickOutside.test.tsx @@ -85,6 +85,28 @@ describe("useClickOutside", () => { document.removeEventListener("keydown", onKey); }); + it("leaves Escape to a confirm opened above it, and still takes it inside a modal", () => { + const { unmount } = render( + <> + +
+ , + ); + toggle("A"); + fireEvent.keyDown(document.body, { key: "Escape" }); + expect(isOpen("A")).toBe(true); + unmount(); + + render( +
+ +
, + ); + toggle("B"); + fireEvent.keyDown(document.body, { key: "Escape" }); + expect(isOpen("B")).toBe(false); + }); + it("closes the open dropdown when another one opens", () => { render( <> diff --git a/web/src/hooks/useClickOutside.ts b/web/src/hooks/useClickOutside.ts index b4d328c90..716a0313b 100644 --- a/web/src/hooks/useClickOutside.ts +++ b/web/src/hooks/useClickOutside.ts @@ -60,6 +60,10 @@ export default function useClickOutside(open: boolean, onClose: () => void, insi // drawer holding it stays open. Focus inside goes back to where it was opened. const onKey = (e: KeyboardEvent) => { if (e.key !== "Escape" || layers[layers.length - 1] !== self) return; + // A modal opened above this layer (the confirm, a dialog) owns Escape first. + const modals = document.querySelectorAll('[role="alertdialog"], [aria-modal="true"]'); + const above = Array.from(modals).some((m) => !refs.current.some((r) => r.current && m.contains(r.current))); + if (above) return; e.stopPropagation(); const active = document.activeElement; const origin = refs.current[0]?.current; diff --git a/web/src/lib/api/hooks/app/advisor/useAdvisor.ts b/web/src/lib/api/hooks/app/advisor/useAdvisor.ts index 313ecfb75..b42488748 100644 --- a/web/src/lib/api/hooks/app/advisor/useAdvisor.ts +++ b/web/src/lib/api/hooks/app/advisor/useAdvisor.ts @@ -133,7 +133,7 @@ export function useSnoozeAdvisorFinding() { }); } -export function useClickOutsideAdvisorFinding() { +export function useDismissAdvisorFinding() { const invalidate = useAdvisorInvalidator(); return useMutation({ mutationFn: ({ id, reason }: { id: string; reason: string }) => dismissAdvisorFinding(id, reason), diff --git a/web/src/lib/api/hooks/app/emails/useMailboxImportActions.ts b/web/src/lib/api/hooks/app/emails/useMailboxImportActions.ts index 7f24cb5ea..7fdd2a8dd 100644 --- a/web/src/lib/api/hooks/app/emails/useMailboxImportActions.ts +++ b/web/src/lib/api/hooks/app/emails/useMailboxImportActions.ts @@ -51,7 +51,7 @@ export function useCancelMailboxImport(id: string | null) { return useJobMutation(id, (i) => cancelMailboxImport(i)); } -export function useClickOutsideMailboxImport() { +export function useDismissMailboxImport() { const qc = useQueryClient(); return useMutation({ mutationFn: (id: string) => dismissMailboxImport(id),