Merge pull request #773 from dealerweb/fix/mailto-unsaved-draft-guard

fix: route mailto composer replacement through the close guard

components/email/__tests__/composer-close-guard.test.tsx

@@ -0,0 +1,314 @@
+import { render, screen, fireEvent, waitFor, act, within } from '@testing-library/react';
+import { describe, it, expect, vi } from 'vitest';
+import React from 'react';
+import { EmailComposer } from '../email-composer';
+import { useAuthStore } from '@/stores/auth-store';
+
+// ─── Heavy component mocks (mirrors reply-addressing.test.tsx) ────────────────
+
+vi.mock('@/components/email/rich-text-editor', () => ({
+  RichTextEditor: () => React.createElement('div', { 'data-testid': 'rich-text-editor' }),
+}));
+
+vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: () => null }));
+vi.mock('@/components/identity/sub-address-helper', () => ({ SubAddressHelper: () => null }));
+vi.mock('@/components/templates/template-picker', () => ({ TemplatePicker: () => null }));
+vi.mock('@/components/templates/template-form', () => ({ TemplateForm: () => null }));
+vi.mock('@/components/files/file-preview-modal', () => ({ FilePreviewModal: () => null }));
+vi.mock('@/hooks/use-focus-trap', () => ({
+  useFocusTrap: () => ({ current: null }),
+}));
+vi.mock('@/hooks/use-pro-multi-account-identities', () => ({
+  useProMultiAccountIdentities: () => ({ enabled: false, groups: [], allIdentities: [] }),
+  stripCrossAccountIdentityPrefix: (id: string) => ({ localAccountId: null, rawId: id }),
+}));
+
+// ─── Store mocks ──────────────────────────────────────────────────────────────
+
+vi.mock('@/stores/auth-store', () => {
+  const state = {
+    client: null,
+    identities: [],
+    primaryIdentity: null,
+    isAuthenticated: false,
+    isDemoMode: false,
+    activeAccountId: null,
+    connectionLost: false,
+    getClientForAccount: () => undefined,
+    getAllConnectedClients: () => new Map(),
+    syncIdentities: () => {},
+    refreshIdentities: async () => {},
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useAuthStore: hook };
+});
+
+vi.mock('@/stores/identity-store', () => {
+  const state = {
+    identities: [{ id: 'id-me', email: 'me@example.com', name: 'Me' }],
+    defaultIdentityId: 'id-me',
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useIdentityStore: hook };
+});
+
+vi.mock('@/stores/account-store', () => {
+  const state = { accounts: [], getAccountById: () => undefined };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useAccountStore: hook };
+});
+
+vi.mock('@/stores/email-store', () => {
+  const state = {
+    draftSaveEnabled: false,
+    sendRawEmail: async () => ({ sent: true }),
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useEmailStore: hook };
+});
+
+const updateSetting = vi.fn();
+
+vi.mock('@/stores/settings-store', () => {
+  const state = {
+    timeFormat: '24h',
+    plainTextMode: false,
+    subAddressDelimiter: '+',
+    autoSelectReplyIdentity: true,
+    attachmentReminderEnabled: false,
+    attachmentReminderKeywords: [],
+    emptySubjectWarningEnabled: true,
+    sendDelaySeconds: 0,
+    signaturePosition: 'above_quote',
+    signatureSeparatorEnabled: false,
+    requestReadReceiptDefault: false,
+    addTrustedSender: () => {},
+    trustedSendersAddressBook: null,
+    updateSetting: (...args: unknown[]) => updateSetting(...args),
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useSettingsStore: hook };
+});
+
+vi.mock('@/stores/contact-store', () => {
+  const state = {
+    contacts: [],
+    getAutocomplete: async () => [],
+    addToTrustedSendersBook: async () => {},
+  };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useContactStore: hook };
+});
+
+vi.mock('@/stores/template-store', () => {
+  const state = { templates: [], addTemplate: async () => {} };
+  const hook = (sel?: (s: typeof state) => unknown) =>
+    typeof sel === 'function' ? sel(state) : state;
+  hook.getState = () => state;
+  hook.setState = (p: Partial<typeof state>) => Object.assign(state, p);
+  return { useTemplateStore: hook };
+});
+
+// ─── Misc dependency mocks ────────────────────────────────────────────────────
+
+vi.mock('@/stores/toast-store', () => ({
+  toast: { info: () => {}, error: () => {}, success: () => {} },
+}));
+
+vi.mock('@/lib/plugin-hooks', () => ({
+  emailHooks: {
+    onComposerOpen: { call: async () => [] },
+    onRecipientChange: { call: async () => [] },
+    getRecipientSuggestions: { call: async () => [] },
+    onRecipientChipsChange: { transform: async (chips: unknown) => chips },
+    onDraftChange: { emit: () => {} },
+    onBeforeDraftAutoSave: { transform: async (draft: unknown) => draft },
+    onBeforeEmailSend: { intercept: async () => true },
+    onComposeSend: { intercept: async () => true },
+    onTransformOutgoingEmail: { transform: async (email: unknown) => email },
+  },
+  contactHooks: {
+    search: { call: async () => [] },
+    onProvideRecipientSuggestions: { transform: async (initial: unknown) => initial },
+  },
+}));
+
+vi.mock('@/lib/email-sanitization', () => ({
+  sanitizeSignatureHtml: (v: string) => v,
+  sanitizeEmailHtml: (v: string) => v,
+  parseHtmlSafely: (html: string) => new DOMParser().parseFromString(html, 'text/html'),
+}));
+
+vi.mock('@/lib/email-threading', () => ({
+  computeReplyThreadingHeaders: () => ({ inReplyTo: [], references: [] }),
+}));
+vi.mock('@/lib/signature-utils', () => ({
+  appendPlainTextSignature: (body: string) => body,
+  getPlainTextSignature: () => '',
+}));
+vi.mock('@/lib/sub-addressing', () => ({ generateSubAddress: () => '' }));
+vi.mock('@/lib/debug', () => ({ debug: { log: () => {}, warn: () => {}, error: () => {} } }));
+vi.mock('@/components/email/quoted-html', () => ({
+  buildQuotedHtmlBlock: () => '',
+  serializeEditorContent: () => '',
+}));
+vi.mock('@/lib/template-utils', () => ({ substitutePlaceholders: (s: string) => s }));
+
+// ─── Tests ────────────────────────────────────────────────────────────────────
+
+/**
+ * A host that replaces the compose session - a mailto: click, say - must not
+ * drop text the user has not sent. It hands a follow-up to requestCloseRef and
+ * the composer runs it only once it has really closed, so "Save" and "Discard"
+ * continue into the new session while "Cancel" leaves the draft standing.
+ */
+
+const DIRTY_DRAFT = {
+  to: 'bob@example.com',
+  cc: '',
+  bcc: '',
+  subject: 'Half-written',
+  body: '<p>Still typing</p>',
+  showCc: false,
+  showBcc: false,
+  selectedIdentityId: 'id-me',
+  subAddressTag: '',
+  mode: 'compose' as const,
+  draftId: null,
+};
+
+function renderWithCloseRef(props: Record<string, unknown> = {}) {
+  const requestCloseRef = { current: null } as React.MutableRefObject<((afterClose?: () => void) => void) | null>;
+  const afterClose = vi.fn();
+  const onClose = vi.fn();
+  const view = render(
+    <EmailComposer
+      initialData={DIRTY_DRAFT}
+      requestCloseRef={requestCloseRef}
+      onClose={onClose}
+      {...props}
+    />,
+  );
+  // Typing is what makes the draft dirty; initialData alone is the baseline.
+  fireEvent.change(screen.getByDisplayValue('Half-written'), {
+    target: { value: 'Half-written more' },
+  });
+  return { requestCloseRef, afterClose, onClose, unmount: view.unmount };
+}
+
+describe('composer close guard with a follow-up', () => {
+  it('asks before replacing an unsaved draft instead of dropping it', async () => {
+    const { requestCloseRef, afterClose } = renderWithCloseRef();
+
+    act(() => requestCloseRef.current!(afterClose));
+
+    expect(await screen.findByText('close_draft_title')).toBeInTheDocument();
+    expect(afterClose).not.toHaveBeenCalled();
+  });
+
+  it('runs the follow-up after discarding', async () => {
+    const { requestCloseRef, afterClose, onClose } = renderWithCloseRef();
+
+    act(() => requestCloseRef.current!(afterClose));
+    const dialog = await screen.findByRole('alertdialog');
+    fireEvent.click(within(dialog).getByText('discard'));
+
+    await waitFor(() => expect(afterClose).toHaveBeenCalledTimes(1));
+    // The host's own close runs first, so the new session starts from a clean slate.
+    expect(onClose).toHaveBeenCalledTimes(1);
+    expect(onClose.mock.invocationCallOrder[0]).toBeLessThan(afterClose.mock.invocationCallOrder[0]);
+  });
+
+  it('drops the follow-up when the user cancels', async () => {
+    const { requestCloseRef, afterClose, onClose } = renderWithCloseRef();
+
+    act(() => requestCloseRef.current!(afterClose));
+    const dialog = await screen.findByRole('alertdialog');
+    fireEvent.click(within(dialog).getByText('cancel'));
+
+    expect(afterClose).not.toHaveBeenCalled();
+    expect(onClose).not.toHaveBeenCalled();
+
+    // A later, unrelated close must not resurrect the abandoned follow-up.
+    act(() => requestCloseRef.current!());
+    const reopened = await screen.findByRole('alertdialog');
+    fireEvent.click(within(reopened).getByText('discard'));
+
+    await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1));
+    expect(afterClose).not.toHaveBeenCalled();
+  });
+
+  it('closes straight through when there is nothing to lose', async () => {
+    const requestCloseRef = { current: null } as React.MutableRefObject<((afterClose?: () => void) => void) | null>;
+    const afterClose = vi.fn();
+    render(<EmailComposer requestCloseRef={requestCloseRef} onClose={vi.fn()} />);
+
+    act(() => requestCloseRef.current!(afterClose));
+
+    await waitFor(() => expect(afterClose).toHaveBeenCalledTimes(1));
+    expect(screen.queryByText('close_draft_title')).not.toBeInTheDocument();
+  });
+
+  it('runs the follow-up after saving the draft', async () => {
+    const { requestCloseRef, afterClose } = renderWithCloseRef();
+
+    act(() => requestCloseRef.current!(afterClose));
+    const dialog = await screen.findByRole('alertdialog');
+    fireEvent.click(within(dialog).getByText('save'));
+
+    await waitFor(() => expect(afterClose).toHaveBeenCalledTimes(1));
+  });
+
+  it('drops the follow-up when the save fails and the rescue takes over', async () => {
+    // When the server refuses the draft, the composer text survives only via
+    // the unmount stash into the continue-draft slot (#702). That rescue
+    // outranks the request that triggered the close, so the follow-up must
+    // never fire - otherwise the new session would bury the stashed text.
+    const createDraft = vi.fn().mockRejectedValue(new Error('server refused'));
+    useAuthStore.setState({
+      client: { createDraft, hasDelayedSend: () => false, getMaxDelayedSend: () => 0 } as never,
+    });
+    const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {});
+    try {
+      const onSaveState = vi.fn();
+      const { requestCloseRef, afterClose, onClose, unmount } = renderWithCloseRef({ onSaveState });
+
+      act(() => requestCloseRef.current!(afterClose));
+      const dialog = await screen.findByRole('alertdialog');
+      fireEvent.click(within(dialog).getByText('save'));
+
+      // The host still gets its close - the composer must not stay stuck -
+      // but the follow-up is dropped because the text was never persisted.
+      await waitFor(() => expect(onClose).toHaveBeenCalledTimes(1));
+      expect(createDraft).toHaveBeenCalled();
+      expect(afterClose).not.toHaveBeenCalled();
+
+      // The rescue itself: unmounting stashes the live text on the host.
+      unmount();
+      expect(onSaveState).toHaveBeenCalledWith(
+        expect.objectContaining({ subject: 'Half-written more' }),
+      );
+    } finally {
+      useAuthStore.setState({ client: null });
+      consoleError.mockRestore();
+    }
+  });
+});

components/email/email-composer.tsx

@@ -155,8 +155,12 @@ interface EmailComposerProps {
    * handler shows the unsaved-changes dialog when the draft is dirty, so a
    * host (e.g. the Pro tab bar's close button) can route an external close
    * request through the same guard instead of discarding silently.
+   *
+   * A host replacing this session with another one (e.g. a mailto: click)
+   * can pass a follow-up that runs once the composer has actually closed.
+   * Cancelling the dialog drops it, so the draft simply stays put.
    */
-  requestCloseRef?: React.MutableRefObject<(() => void) | null>;
+  requestCloseRef?: React.MutableRefObject<((afterClose?: () => void) => void) | null>;
   onDiscardDraft?: (draftId: string) => void;
   onSaveState?: (data: ComposerDraftData) => void;
   className?: string;
@@ -575,7 +579,7 @@ export function EmailComposer({
 
   const closeDialogRef = useFocusTrap({
     isActive: showCloseDialog,
-    onEscape: () => setShowCloseDialog(false),
+    onEscape: () => dismissCloseDialog(),
     restoreFocus: true,
   });
 
@@ -2147,6 +2151,29 @@ export function EmailComposer({
     handleSend({ delayedUntil: new Date(scheduleValue).toISOString() });
   };
 
+  // A host can hand a follow-up to requestCloseRef - "close this session, then
+  // do that". It runs only once the composer really closes, never when the
+  // guard dialog is cancelled, and never before onClose has had its say.
+  const afterCloseRef = useRef<(() => void) | null>(null);
+
+  const emitClose = () => {
+    onClose?.();
+    const afterClose = afterCloseRef.current;
+    afterCloseRef.current = null;
+    // A save that the server refused resets explicitCloseRef so the unmount
+    // stash hands the text back to the host's continue-draft slot (#702). The
+    // draft is still alive in that case, so a follow-up that would replace it
+    // must not run - the rescue outranks the request that triggered it.
+    if (explicitCloseRef.current) {
+      afterClose?.();
+    }
+  };
+
+  const dismissCloseDialog = () => {
+    afterCloseRef.current = null;
+    setShowCloseDialog(false);
+  };
+
   const cleanClose = () => {
     sendCancelledRef.current = true;
     explicitCloseRef.current = true;
@@ -2154,7 +2181,7 @@ export function EmailComposer({
       clearTimeout(saveTimeoutRef.current);
     }
     stateRef.current = { to: '', cc: '', bcc: '', subject: '', body: '', showCc: false, showBcc: false, selectedIdentityId: null, subAddressTag: '', draftId: null, fromOverrideEnabled: false, fromOverrideEmail: '', fromOverrideName: '' };
-    onClose?.();
+    emitClose();
   };
 
   const handleSaveDraftAndClose = async () => {
@@ -2181,7 +2208,7 @@ export function EmailComposer({
         stateRef.current = { to: '', cc: '', bcc: '', subject: '', body: '', showCc: false, showBcc: false, selectedIdentityId: null, subAddressTag: '', draftId: null, fromOverrideEnabled: false, fromOverrideEmail: '', fromOverrideName: '' };
       }
     } finally {
-      onClose?.();
+      emitClose();
     }
   };
 
@@ -2196,10 +2223,11 @@ export function EmailComposer({
       onDiscardDraft(draftId);
     }
     stateRef.current = { to: '', cc: '', bcc: '', subject: '', body: '', showCc: false, showBcc: false, selectedIdentityId: null, subAddressTag: '', draftId: null, fromOverrideEnabled: false, fromOverrideEmail: '', fromOverrideName: '' };
-    onClose?.();
+    emitClose();
   };
 
-  const handleClose = () => {
+  const handleClose = (afterClose?: () => void) => {
+    afterCloseRef.current = afterClose ?? null;
     if (isDirtyRef.current) {
       setShowCloseDialog(true);
     } else {
@@ -2291,7 +2319,7 @@ export function EmailComposer({
       {/* Header - mobile: clean bar with close/send, desktop: title bar */}
       <div className="flex items-center justify-between px-4 py-3 border-b bg-background">
         <div className="flex items-center gap-3">
-          <Button variant="ghost" size="icon" onClick={handleClose} className="h-9 w-9 md:h-8 md:w-8">
+          <Button variant="ghost" size="icon" onClick={() => handleClose()} className="h-9 w-9 md:h-8 md:w-8">
             <X className="w-5 h-5 md:w-4 md:h-4" />
           </Button>
           <div className="flex items-center gap-2" data-testid="composer-save-status" data-status={saveStatus}>
@@ -2845,7 +2873,7 @@ export function EmailComposer({
           <div className="flex items-center gap-2">
             <button
               type="button"
-              onClick={handleClose}
+              onClick={() => handleClose()}
               className="text-sm text-muted-foreground hover:text-red-500 transition-colors px-2 py-1"
             >
               {t('discard')}
@@ -3049,7 +3077,7 @@ export function EmailComposer({
       {showCloseDialog && (
         <div
           className="fixed inset-0 bg-black/50 backdrop-blur-[1px] flex items-center justify-center z-[60] p-4 animate-in fade-in duration-150"
-          onClick={() => setShowCloseDialog(false)}
+          onClick={() => dismissCloseDialog()}
         >
           <div
             ref={closeDialogRef}
@@ -3063,7 +3091,7 @@ export function EmailComposer({
               <p className="mt-2 text-sm text-muted-foreground">{t('close_draft_message')}</p>
             </div>
             <div className="flex items-center justify-end gap-3 px-6 pb-6">
-              <Button variant="outline" onClick={() => setShowCloseDialog(false)}>
+              <Button variant="outline" onClick={() => dismissCloseDialog()}>
                 {t('cancel')}
               </Button>
               <Button variant="destructive" onClick={handleDiscardAndClose}>

components/mail/mail-app.tsx

@@ -119,6 +119,10 @@ export function MailApp({ linkSegments }: MailAppProps = {}) {
   // Cleared on close so a subsequent "compose new" doesn't reuse stale state.
   const [composerQuoteHeader, setComposerQuoteHeader] = useState<QuoteHeader | null>(null);
   const suppressComposerStateSaveSessionRef = useRef<number | null>(null);
+  // The inline composer publishes its dirty-aware close here (the Pro tab bar
+  // already uses the same seam), so entry points that replace the session can
+  // ask before discarding unsaved text.
+  const composerRequestCloseRef = useRef<((afterClose?: () => void) => void) | null>(null);
   const { dialogProps: confirmDialogProps, confirm: confirmDialog } = useConfirmDialog();
   const { dialogProps: promptDialogProps, prompt: promptDialog } = usePromptDialog();
   const { showAppsModal, inlineApp, loadedApps, handleManageApps, handleInlineApp, closeInlineApp, closeAppsModal } = useSidebarApps();
@@ -958,12 +962,12 @@ export function MailApp({ linkSegments }: MailAppProps = {}) {
     }
   }, [initialCheckDone, isAuthenticated, authLoading]);
 
-  const openMailtoDraft = useCallback((pending: ParsedMailto) => {
+  const applyMailtoDraft = useCallback((pending: ParsedMailto, suppressOutgoingStash: boolean) => {
     const body = useSettingsStore.getState().plainTextMode
       ? pending.body
       : plainTextToComposerBody(pending.body);
 
-    if (showComposer) {
+    if (suppressOutgoingStash) {
       suppressComposerStateSaveSessionRef.current = composerSessionId;
     }
     setComposerSessionId((id) => id + 1);
@@ -983,7 +987,24 @@ export function MailApp({ linkSegments }: MailAppProps = {}) {
     setComposerMode("compose");
     setShowComposer(true);
     if (isMobile) setActiveView("viewer");
-  }, [composerSessionId, isMobile, setActiveView, showComposer]);
+  }, [composerSessionId, isMobile, setActiveView]);
+
+  const openMailtoDraft = useCallback((pending: ParsedMailto) => {
+    // A live composer owns text the user has not sent. Replacing it outright
+    // used to drop that text on the floor - the stash suppression below is an
+    // ordering fix, not a decision anyone made. Route the request through the
+    // composer's own "Save or discard draft?" guard instead and open the
+    // mailto draft only once it has actually closed; cancelling leaves the
+    // draft alone. No suppression on that path: when a save fails the composer
+    // deliberately hands its text back to the continue-draft slot (#702), and
+    // that rescue must win over this request.
+    const requestClose = composerRequestCloseRef.current;
+    if (showComposer && requestClose) {
+      requestClose(() => applyMailtoDraft(pending, false));
+      return;
+    }
+    applyMailtoDraft(pending, showComposer);
+  }, [applyMailtoDraft, showComposer]);
 
   const openMailtoForAccount = useCallback(async (pending: ParsedMailto, accountId: string) => {
     setIsProtocolAccountSwitching(true);
@@ -3797,6 +3818,7 @@ export function MailApp({ linkSegments }: MailAppProps = {}) {
                       setActiveView('list');
                     }
                   }}
+                  requestCloseRef={composerRequestCloseRef}
                   onDiscardDraft={(draftId) => {
                     handleDiscardDraft(draftId);
                     setPendingDraft(null);

components/pro/pro-compose-tab-body.tsx

@@ -40,7 +40,7 @@ export function ProComposeTabBody({ tabId, data }: ProComposeTabBodyProps) {
 
   // Set by the composer to its dirty-aware close handler. Lets the Pro tab
   // bar's "X" route through the same "Save or discard draft?" guard.
-  const requestCloseRef = useRef<(() => void) | null>(null);
+  const requestCloseRef = useRef<((afterClose?: () => void) => void) | null>(null);
 
   const handleScheduledSendCreated = useCallback(async () => {
     if (client) {