From 382b26f128da545691203d1c472903322c8fd332 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 28 Oct 2026 12:49:15 +0100 Subject: [PATCH] fix: ask before the rail or the mountain view top bar leaves an event or task with unsaved changes --- .../calendar/__tests__/calendar-app.test.tsx | 35 ++++++- components/calendar/calendar-app.tsx | 18 +++- .../__tests__/mv-top-bar-leave-guard.test.tsx | 95 +++++++++++++++++++ components/layout/mv-top-bar.tsx | 23 +++-- lib/leave-guard.ts | 20 +++- 5 files changed, 179 insertions(+), 12 deletions(-) create mode 100644 components/layout/__tests__/mv-top-bar-leave-guard.test.tsx diff --git a/components/calendar/__tests__/calendar-app.test.tsx b/components/calendar/__tests__/calendar-app.test.tsx index 4b36fb2f2..f8d89b65a 100644 --- a/components/calendar/__tests__/calendar-app.test.tsx +++ b/components/calendar/__tests__/calendar-app.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, fireEvent, within, waitFor } from '@testing-library/react'; +import { act, render, screen, fireEvent, within, waitFor } from '@testing-library/react'; import { describe, it, expect, vi, beforeEach } from 'vitest'; import type { Calendar, CalendarEvent } from '@/lib/jmap/types'; import { useCalendarStore } from '@/stores/calendar-store'; @@ -31,6 +31,7 @@ vi.mock('@/hooks/use-config', () => ({ useConfig: () => config })); import { CalendarApp } from '../calendar-app'; import { useRegisteredCommandGroups } from '@/lib/command-registry'; +import { interceptLeave } from '@/lib/leave-guard'; // The first render loads the whole page and is slow under load. vi.setConfig({ testTimeout: 30000 }); @@ -273,6 +274,38 @@ describe('CalendarApp unsaved editor changes', () => { fireEvent.click(within(ask).getByRole('button', { name: 'discard.confirm' })); await waitFor(() => expect(screen.queryByPlaceholderText('tasks.title_placeholder')).toBeNull()); }); + + it('has the rail ask before it leaves an editor with changes', async () => { + renderApp(); + // An untouched editor lets the rail go at once. + fireEvent.keyDown(document.body, { key: 'n', code: 'KeyN' }); + const navigated = vi.fn(); + // A rail link wired as the real one is: interceptLeave first. + const link = document.createElement('a'); + link.addEventListener('click', (e) => { if (!interceptLeave(e)) navigated(); }); + document.body.appendChild(link); + try { + act(() => { link.click(); }); + expect(navigated).toHaveBeenCalledOnce(); + expect(screen.queryByRole('alertdialog')).toBeNull(); + + fireEvent.change(screen.getByPlaceholderText('form.title'), { target: { value: 'Lunch' } }); + act(() => { link.click(); }); + const ask = await screen.findByRole('alertdialog', { name: 'discard.title' }); + expect(navigated).toHaveBeenCalledOnce(); + fireEvent.click(within(ask).getByRole('button', { name: 'discard.keep' })); + await waitFor(() => expect(screen.queryByRole('alertdialog')).toBeNull()); + expect(screen.getByPlaceholderText('form.title')).toHaveValue('Lunch'); + expect(navigated).toHaveBeenCalledOnce(); + + act(() => { link.click(); }); + const again = await screen.findByRole('alertdialog', { name: 'discard.title' }); + fireEvent.click(within(again).getByRole('button', { name: 'discard.confirm' })); + await waitFor(() => expect(navigated).toHaveBeenCalledTimes(2)); + } finally { + link.remove(); + } + }); }); describe('CalendarApp browser Back with an editor open (R5)', () => { diff --git a/components/calendar/calendar-app.tsx b/components/calendar/calendar-app.tsx index 6b29b5ea0..3fb193e68 100644 --- a/components/calendar/calendar-app.tsx +++ b/components/calendar/calendar-app.tsx @@ -87,6 +87,7 @@ import { appPath, buildCalendarPath, parseCalendarPath, type CalendarDeepLink } import { consumePendingDeepLinkEntry, subscribePendingDeepLink } from "@/lib/deep-link-handoff"; import { useDeepLinkUrl } from "@/hooks/use-deep-link-url"; import { useCloseOnBack } from "@/hooks/use-close-on-back"; +import { useLeaveGuard } from "@/lib/leave-guard"; import { useCalendarEventSearch } from "@/hooks/use-calendar-event-search"; import { CalendarSearchField, CalendarSearchResults } from "@/components/calendar/calendar-search"; import { useProInterfaceActive } from "@/components/pro/pro-interface-redirect"; @@ -966,11 +967,26 @@ export function CalendarApp({ linkSegments: routeSegments }: CalendarAppProps = setShowTaskModal(false); setEditTask(null); }, []); + const editorsInView = (showEventModal || showTaskModal) && (isEmbedded ? isFocusedProTab : !proInterfaceActive); useCloseOnBack({ - active: (showEventModal || showTaskModal) && (isEmbedded ? isFocusedProTab : !proInterfaceActive), + active: editorsInView, shouldHold: () => eventEditorDirtyRef.current || taskEditorDirtyRef.current, onBack: () => whenEditorsDiscarded(closeEditors), }); + // The rail, the phone tab bar and the Mountain View top bar ask too, + // before they leave the calendar (R5). Only ask there: leaving unmounts + // the editor, and clearing it first would let the Back guard above take + // its history step back over the navigation. + useLeaveGuard( + () => editorsInView && (eventEditorDirtyRef.current || taskEditorDirtyRef.current), + () => confirmAction({ + title: t("discard.title"), + message: t("discard.message"), + confirmText: t("discard.confirm"), + cancelText: t("discard.keep"), + variant: "destructive", + }), + ); const handleEditFromDetail = useCallback(() => { if (detailEvent) { diff --git a/components/layout/__tests__/mv-top-bar-leave-guard.test.tsx b/components/layout/__tests__/mv-top-bar-leave-guard.test.tsx new file mode 100644 index 000000000..b817661a2 --- /dev/null +++ b/components/layout/__tests__/mv-top-bar-leave-guard.test.tsx @@ -0,0 +1,95 @@ +import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type React from 'react'; + +// R5: an editor with unsaved changes asks before Mountain View's top bar +// leaves it, as the rail and the phone tab bar do. + +const nav = vi.hoisted(() => ({ push: vi.fn(), followed: vi.fn() })); +const switcher = vi.hoisted(() => ({ onSelectAllInboxes: undefined as undefined | (() => void) })); + +vi.mock('../account-switcher', () => ({ + AccountSwitcher: (props: { onSelectAllInboxes?: () => void }) => { + switcher.onSelectAllInboxes = props.onSelectAllInboxes; + return null; + }, +})); +vi.mock('@/hooks/use-config', () => ({ useConfig: () => ({ appName: 'Mail' }) })); +vi.mock('@/hooks/use-stalwart-admin', () => ({ useStalwartAdmin: () => false })); +vi.mock('@/i18n/navigation', () => ({ + useRouter: () => ({ push: nav.push, back: vi.fn(), replace: vi.fn() }), + usePathname: () => '/calendar', + // Like next/link: the handler runs first, and a prevented click stays put. + Link: ({ href, onClick, children, ...rest }: React.AnchorHTMLAttributes & { href: string }) => ( + { + onClick?.(e); + if (!e.defaultPrevented) nav.followed(href); + e.preventDefault(); + }} + > + {children} + + ), +})); + +const { MvTopBar } = await import('../mv-top-bar'); +const { registerLeaveGuard } = await import('@/lib/leave-guard'); +const { useSettingsStore } = await import('@/stores/settings-store'); +const { useAccountStore } = await import('@/stores/account-store'); +const { usePolicyStore } = await import('@/stores/policy-store'); +const { DEFAULT_POLICY } = await import('@/lib/admin/types'); + +let unregister: (() => void) | null = null; +let confirm: ReturnType Promise>>; + +beforeEach(() => { + nav.push.mockClear(); + nav.followed.mockClear(); + usePolicyStore.setState({ policy: { ...DEFAULT_POLICY, features: { ...DEFAULT_POLICY.features, unifiedCrossAccountEnabled: true } } }); + useSettingsStore.setState({ enableUnifiedMailbox: true, unifiedCrossAccount: true } as never); + useAccountStore.setState({ accounts: [{ id: 'a', isConnected: true }, { id: 'b', isConnected: true }] } as never); + confirm = vi.fn(async () => true); + unregister = registerLeaveGuard({ isDirty: () => true, confirm }); +}); +afterEach(() => { unregister?.(); unregister = null; }); + +describe('Mountain View top bar with unsaved changes', () => { + it('asks before the Settings button leaves, then goes on', async () => { + render(); + fireEvent.click(screen.getByRole('link', { name: 'settings' })); + expect(confirm).toHaveBeenCalledOnce(); + expect(nav.followed).not.toHaveBeenCalled(); + await waitFor(() => expect(nav.followed).toHaveBeenCalledWith('/settings')); + }); + + it('stays when the user keeps editing', async () => { + confirm.mockResolvedValue(false); + render(); + fireEvent.click(screen.getByRole('link', { name: 'Mail' })); + await waitFor(() => expect(confirm).toHaveBeenCalledOnce()); + await act(async () => { await new Promise((r) => setTimeout(r, 10)); }); + expect(nav.followed).not.toHaveBeenCalled(); + expect(nav.push).not.toHaveBeenCalled(); + }); + + it('asks before an app in the apps menu, and opens it after the menu has closed', async () => { + render(); + fireEvent.click(screen.getByRole('button', { name: 'add_app' })); + fireEvent.click(screen.getByRole('menuitem', { name: 'mail' })); + expect(confirm).toHaveBeenCalledOnce(); + expect(nav.followed).not.toHaveBeenCalled(); + expect(screen.queryByRole('menu')).toBeNull(); + await waitFor(() => expect(nav.push).toHaveBeenCalledWith('/')); + }); + + it('asks before "All inboxes" opens mail', async () => { + render(); + act(() => switcher.onSelectAllInboxes!()); + expect(confirm).toHaveBeenCalledOnce(); + expect(nav.push).not.toHaveBeenCalled(); + await waitFor(() => expect(nav.push).toHaveBeenCalledWith('/')); + }); +}); diff --git a/components/layout/mv-top-bar.tsx b/components/layout/mv-top-bar.tsx index 9694e6763..a548233df 100644 --- a/components/layout/mv-top-bar.tsx +++ b/components/layout/mv-top-bar.tsx @@ -31,6 +31,7 @@ import { getPathPrefix, withBasePath } from "@/lib/browser-navigation"; import { useStalwartAdmin } from "@/hooks/use-stalwart-admin"; import { useUpdateStore, selectHasUpdate } from "@/stores/update-store"; import { cn } from "@/lib/utils"; +import { interceptLeave, leaveWhenAllowed } from "@/lib/leave-guard"; import { openKeyboardShortcuts, parkAppCommand, requestAppCommand } from "@/lib/app-commands"; import { useSettingsStore } from "@/stores/settings-store"; import { useAccountStore } from "@/stores/account-store"; @@ -127,8 +128,10 @@ export function MvTopBar({ const selectAllInboxes = onSelectAllInboxes ?? (allInboxesReachable ? () => { if (requestAppCommand("all-inboxes")) return; - parkAppCommand("all-inboxes"); - router.push("/"); + leaveWhenAllowed(() => { + parkAppCommand("all-inboxes"); + router.push("/"); + }); } : undefined); const { appName, appLogoLightUrl, appLogoDarkUrl } = useConfig(); @@ -213,7 +216,7 @@ export function MvTopBar({ )} - + { interceptLeave(e); }} className="flex items-center gap-2 pe-4 min-w-0" aria-label={appName}> {logoUrl ? : null} {appName} @@ -288,6 +291,7 @@ export function MvTopBar({ {isStalwartAdmin && ( { interceptLeave(e); }} className="relative grid place-items-center w-12 h-12 rounded-full text-muted-foreground hover:bg-muted hover:text-foreground transition-colors" title={t("admin")} aria-label={hasUpdate ? `${t("admin")}, ${tShell(updateImportant ? "update_important" : "update_available")}` : t("admin")} @@ -306,6 +310,7 @@ export function MvTopBar({ )} { interceptLeave(e); }} className="grid place-items-center w-12 h-12 rounded-full text-muted-foreground hover:bg-muted hover:text-foreground transition-colors" title={t("settings")} aria-label={t("settings")} @@ -347,8 +352,11 @@ export function MvTopBar({ key={app.id} href={app.href} role="menuitem" - onClick={() => { - if (activeAppId) onCloseInlineApp?.(); + onClick={(e) => { + // The menu is gone by the time the user answers, + // so the way out is replayed, not re-clicked. + const leave = () => { if (activeAppId) onCloseInlineApp?.(); }; + if (!interceptLeave(e, () => { leave(); router.push(app.href); })) leave(); setAppsOpen(false); }} className={cn( @@ -370,8 +378,9 @@ export function MvTopBar({ role="menuitem" title={page.pluginName} data-plugin-page-link={`${page.pluginId}/${page.id}`} - onClick={() => { - if (activeAppId) onCloseInlineApp?.(); + onClick={(e) => { + const leave = () => { if (activeAppId) onCloseInlineApp?.(); }; + if (!interceptLeave(e, () => { leave(); router.push(pluginPageHref(page)); })) leave(); setAppsOpen(false); }} className={cn( diff --git a/lib/leave-guard.ts b/lib/leave-guard.ts index 6e927031a..ae571410b 100644 --- a/lib/leave-guard.ts +++ b/lib/leave-guard.ts @@ -47,8 +47,14 @@ export async function confirmLeave(): Promise { * when an editor has unsaved changes, stops the click, asks, and clicks the * same element again once the user lets the changes go. Returns true when * it stopped the click; the handler should then do nothing else. + * + * `replay` leaves in place of the second click, for an element that is gone + * by then (an item of a menu that closes). */ -export function interceptLeave(e: { preventDefault(): void; currentTarget: EventTarget | null }): boolean { +export function interceptLeave( + e: { preventDefault(): void; currentTarget: EventTarget | null }, + replay?: () => void, +): boolean { if (replaying || !hasUnsavedChanges()) return false; e.preventDefault(); const target = e.currentTarget instanceof HTMLElement ? e.currentTarget : null; @@ -56,17 +62,25 @@ export function interceptLeave(e: { preventDefault(): void; currentTarget: Event asking = true; void confirmLeave().then(async (leave) => { asking = false; - if (!leave || !target) return; + if (!leave || (!replay && !target)) return; // The dialog that asked steps Back off its own history entry as it // closes; a navigation started before that lands is undone by it. await new Promise((resolve) => setTimeout(resolve, 0)); await historySettled(); replaying = true; - try { target.click(); } finally { replaying = false; } + try { + if (replay) replay(); + else target?.click(); + } finally { replaying = false; } }); return true; } +/** For a way out that isn't a click (a menu command): leaves now, or once the user lets the changes go. */ +export function leaveWhenAllowed(leave: () => void): void { + if (!interceptLeave({ preventDefault() {}, currentTarget: null }, leave)) leave(); +} + /** Registers the calling editor's guard while it is mounted. */ export function useLeaveGuard(isDirty: () => boolean, confirm: () => Promise): void { const latest = useRef({ isDirty, confirm });