From 0f799454e4ba53a01858a1601cfbf26be4ea0433 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:42:41 +0100 Subject: [PATCH] fix: keep text typed after a clean Mountain View composer move and ask before dropping handed-over edits --- .../composer-sign-out-flush.test.tsx | 46 +++++++++++++++++++ components/email/email-composer.tsx | 8 +++- .../mail-app-mv-composer-move.test.tsx | 43 ++++++++++++++++- components/mail/mail-app.tsx | 15 ++---- 4 files changed, 98 insertions(+), 14 deletions(-) diff --git a/components/email/__tests__/composer-sign-out-flush.test.tsx b/components/email/__tests__/composer-sign-out-flush.test.tsx index be454dee1..8fac873cc 100644 --- a/components/email/__tests__/composer-sign-out-flush.test.tsx +++ b/components/email/__tests__/composer-sign-out-flush.test.tsx @@ -246,6 +246,52 @@ describe('composer flush before an automatic sign-out', () => { expect(createDraft.mock.calls[0][1]).toBe('Half-written'); }); + it('asks before closing a composer opened with stashed edits', async () => { + const onClose = vi.fn(); + const requestCloseRef: { current: ((afterClose?: () => void) => void) | null } = { current: null }; + render(); + + act(() => requestCloseRef.current?.()); + + expect(await screen.findByRole('alertdialog')).toBeTruthy(); + expect(onClose).not.toHaveBeenCalled(); + }); + + it('closes a composer opened with text the server already has without asking', () => { + const onClose = vi.fn(); + const requestCloseRef: { current: ((afterClose?: () => void) => void) | null } = { current: null }; + render(); + + act(() => requestCloseRef.current?.()); + + expect(screen.queryByRole('alertdialog')).toBeNull(); + expect(onClose).toHaveBeenCalled(); + }); + + it('autosaves a composer opened with stashed edits', async () => { + render(); + + await waitFor(() => expect(createDraft).toHaveBeenCalledTimes(1), { timeout: 4000 }); + expect(createDraft.mock.calls[0][1]).toBe('Half-written'); + }); + + it('hands nothing back for stashed edits it did not change: the host has them', () => { + const onSaveState = vi.fn(); + const view = render(); + view.unmount(); + + expect(onSaveState).not.toHaveBeenCalled(); + }); + + it('hands back the draft id an autosave of stashed edits got', async () => { + const onSaveState = vi.fn(); + const view = render(); + await act(async () => { await flushComposers(5000); }); + view.unmount(); + + expect(onSaveState).toHaveBeenCalledWith(expect.objectContaining({ draftId: 'draft-1', unsaved: false })); + }); + it('waits for a send in flight and saves no draft of the sent message', async () => { let finishSend!: () => void; const onSend = vi.fn(() => new Promise((resolve) => { finishSend = resolve; })); diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index caacdb928..cf4b1da6a 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -1779,6 +1779,11 @@ export function EmailComposer({ if (priority !== initialPriorityRef.current) isDirtyRef.current = true; const initialLabelChoiceRef = useRef(classification.choice); if (classification.choice !== initialLabelChoiceRef.current) isDirtyRef.current = true; + // Text the host handed in is no edit to hand back, but the server may not have + // it: it still needs the autosave and the question on close, so it counts as dirty. + const hasEditsRef = useRef(false); + hasEditsRef.current = isDirtyRef.current; + if (initialData?.unsaved) isDirtyRef.current = true; const firstLabelRunRef = useRef(true); useEffect(() => { if (firstLabelRunRef.current) { @@ -1839,7 +1844,8 @@ export function EmailComposer({ useEffect(() => { return () => { if (explicitCloseRef.current) return; - if (onSaveState && isDirtyRef.current) { + // Only what the host has no copy of: an edit, or a draft saved under a new id. + if (onSaveState && (hasEditsRef.current || draftIdRef.current !== (initialData?.draftId ?? null))) { const s = stateRef.current; onSaveState({ ...s, diff --git a/components/mail/__tests__/mail-app-mv-composer-move.test.tsx b/components/mail/__tests__/mail-app-mv-composer-move.test.tsx index d4e30832f..117248914 100644 --- a/components/mail/__tests__/mail-app-mv-composer-move.test.tsx +++ b/components/mail/__tests__/mail-app-mv-composer-move.test.tsx @@ -21,7 +21,8 @@ vi.mock('@/components/layout/sidebar', () => ({ Sidebar: ({ onCompose }: { onCompose: () => void }) => , })); // The real composer's contract: its state starts from initialData, and it hands -// its edits to onSaveState as it unmounts. +// its edits to onSaveState as it unmounts, only if it has any, through the +// handler of its first render. Text it started with is no edit. vi.mock('@/components/email/email-composer', async () => { const { useEffect, useRef, useState } = await import('react'); return { @@ -30,10 +31,11 @@ vi.mock('@/components/email/email-composer', async () => { onSaveState: (data: { subject: string }) => void; }) => { const [subject, setSubject] = useState(initialData?.subject ?? ''); + const initial = useRef(subject); const latest = useRef(subject); latest.current = subject; useEffect(() => () => { - if (latest.current) onSaveState({ subject: latest.current }); + if (latest.current !== initial.current) onSaveState({ subject: latest.current }); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); return setSubject(e.target.value)} />; @@ -119,6 +121,43 @@ describe('moving the Mountain View composer between layouts', () => { rerender(); await waitFor(() => expect(screen.getByLabelText('stub subject').value).toBe('Still here')); }); + + const setWidth = (rerender: (ui: React.ReactElement) => void, width: 'desktop' | 'tablet') => { + device.current = { isMobile: false, isTablet: width === 'tablet', isDesktop: width === 'desktop' }; + rerender(); + }; + const subject = () => screen.getByLabelText('stub subject'); + + it('keeps text typed after the composer moved with nothing in it yet', async () => { + const { rerender } = render(); + fireEvent.click(screen.getByText('stub compose')); + await screen.findByLabelText('stub subject'); + + // Clean, so this move hands nothing over. + setWidth(rerender, 'tablet'); + fireEvent.change(await screen.findByLabelText('stub subject'), { target: { value: 'Typed in the pane' } }); + + setWidth(rerender, 'desktop'); + await waitFor(() => expect(subject().value).toBe('Typed in the pane')); + + setWidth(rerender, 'tablet'); + await waitFor(() => expect(subject().value).toBe('Typed in the pane')); + }); + + it('keeps text typed after moving back and forth untouched', async () => { + const { rerender } = render(); + fireEvent.click(screen.getByText('stub compose')); + fireEvent.change(await screen.findByLabelText('stub subject'), { target: { value: 'First' } }); + + setWidth(rerender, 'tablet'); + await waitFor(() => expect(subject().value).toBe('First')); + setWidth(rerender, 'desktop'); + await waitFor(() => expect(subject().value).toBe('First')); + fireEvent.change(subject(), { target: { value: 'First and more' } }); + + setWidth(rerender, 'tablet'); + await waitFor(() => expect(subject().value).toBe('First and more')); + }); }); describe('a composer opened from the phone and tablet drawer', () => { diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 44beafa0b..5299ac7fa 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -268,8 +268,6 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const composerSessionRef = useRef(composerSessionId); composerSessionRef.current = composerSessionId; const composerHostRef = useRef('pane'); - const composerRemountHostRef = useRef('pane'); - const composerRemountSessionRef = useRef(composerSessionId); const showComposerRef = useRef(showComposer); showComposerRef.current = showComposer; // Fields a plugin filled in through onBeforeCompose for the composer about @@ -4576,10 +4574,6 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const floatingComposer = mvShell && showComposer && !isEmbedded && !mvInlineReply; const composerHost = floatingComposer ? 'window' : mvInlineReply ? 'inline' : 'pane'; composerHostRef.current = composerHost; - if (!showComposer || composerRemountSessionRef.current !== composerSessionId) { - composerRemountSessionRef.current = composerSessionId; - composerRemountHostRef.current = composerHost; - } const hasViewerContent = (showComposer && !floatingComposer) || Boolean(conversationThread) || Boolean(selectedEmail && !(mvShell && cursorOnly)) || deepLinkPane !== null; // A linked message on a phone opens over the list, as one picked there does. @@ -4945,11 +4939,10 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { }); return; } - if (showComposerRef.current && composerSessionRef.current === composerSessionId) { - // Still open: it moved to another host, or is the stand-in mounted - // there before the edits arrived. The stand-in's state is out of date. - if (composerHostRef.current === composerRemountHostRef.current) return; - composerRemountHostRef.current = composerHostRef.current; + // Still open in another host: the one mounted there started before + // these edits arrived. The composer calls the handler of its first + // render, so composerHost is the host this instance lived in. + if (showComposerRef.current && composerSessionRef.current === composerSessionId && composerHostRef.current !== composerHost) { setComposerRemount((n) => n + 1); } setPendingDraft(data);