From 0c34edb64e4d50fe79dbafc478c450ebf8294c08 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 16:09:39 +0100 Subject: [PATCH] fix: draw an opened message with its conversation once, and open a message link with its conversation instead of alone first The conversation effect threw away the conversation the open had just put on screen and fetched it again, which drew the whole mail view once more per opened message. A message link showed the message alone, then a placeholder for the conversation, then the message again, moving the body down. The routing test now expects the link's fetch to leave opening to the mail view, which opens the message with its conversation. --- .../mail-app-conversation-pane.test.tsx | 48 ++++++++++++++++--- .../mail-app-source-routing.test.tsx | 3 +- components/mail/mail-app.tsx | 47 ++++++++++++++++-- stores/email-store.ts | 11 +++-- 4 files changed, 94 insertions(+), 15 deletions(-) diff --git a/components/mail/__tests__/mail-app-conversation-pane.test.tsx b/components/mail/__tests__/mail-app-conversation-pane.test.tsx index e26eeca3e..bb762955c 100644 --- a/components/mail/__tests__/mail-app-conversation-pane.test.tsx +++ b/components/mail/__tests__/mail-app-conversation-pane.test.tsx @@ -21,13 +21,19 @@ vi.mock('next/dynamic', () => ({ default: (load: () => Promise; }; } })); -type ViewerProps = { mvConversation?: { id: string }[]; conversationPane?: boolean }; +type ViewerProps = { email?: { id: string } | null; mvConversation?: { id: string }[]; mvConversationPending?: boolean; conversationPane?: boolean }; +// What the reader was drawn with, render by render: message, conversation, waiting. +const readerDraws = vi.hoisted(() => [] as string[]); vi.mock('@/components/email/email-viewer', () => ({ - EmailViewer: ({ mvConversation, conversationPane }: ViewerProps) => ( -
- {mvConversation ? mvConversation.map((e) => e.id).join(',') : 'no conversation'} -
- ), + EmailViewer: ({ email, mvConversation, mvConversationPending, conversationPane }: ViewerProps) => { + const conversation = mvConversation ? mvConversation.map((e) => e.id).join(',') : 'no conversation'; + readerDraws.push(`${email?.id ?? '-'} ${conversation}${mvConversationPending ? ' waiting' : ''}`); + return ( +
+ {conversation} +
+ ); + }, })); vi.mock('@/components/email/email-composer', () => ({ EmailComposer: () => null })); vi.mock('@/components/layout/sidebar', () => ({ Sidebar: () => null })); @@ -124,6 +130,7 @@ beforeEach(() => { fetchEmailContent, } as never); opened = []; + readerDraws.length = 0; unsubscribe = useEmailStore.subscribe((state, prev) => { if (state.selectedEmail !== prev.selectedEmail) opened.push(state.selectedEmail?.id ?? null); }); @@ -155,6 +162,35 @@ describe('The reading pane in the default layout', () => { await waitFor(() => expect(reader).toHaveTextContent('m0,m1')); }); + it('opens a message with its conversation at once, in one draw', async () => { + useEmailStore.setState({ selectedEmail: inbox()[1], listRows: null } as never); + render(); + const reader = await screen.findByTestId('reader', {}, { timeout: 5000 }); + await waitFor(() => expect(reader).toHaveTextContent('no conversation')); + readerDraws.length = 0; + await press('k'); + await waitFor(() => expect(selectedId()).toBe('m1')); + await waitFor(() => expect(reader).toHaveTextContent('m0,m1')); + await act(async () => { await new Promise((resolve) => setTimeout(resolve, 50)); }); + // The open brought the conversation with the message; fetching it again + // after the open drew the whole mail view once more. + expect(readerDraws.filter((draw) => draw.startsWith('m1 '))).toEqual(['m1 m0,m1']); + }); + + it('shows a linked message with its conversation, not alone first', async () => { + useEmailStore.setState({ selectedEmail: null } as never); + fetchEmailContent.mockImplementation(async (_client: unknown, id: string, _owner?: string, options?: { select?: boolean }) => { + const email = { ...inbox().find((e) => e.id === id)!, bodyValues: {} }; + if (options?.select !== false) useEmailStore.setState({ selectedEmail: email } as never); + return email; + }); + render(); + const reader = await screen.findByTestId('reader', {}, { timeout: 5000 }); + await waitFor(() => expect(reader).toHaveTextContent('m0,m1'), { timeout: 5000 }); + await act(async () => { await new Promise((resolve) => setTimeout(resolve, 50)); }); + expect(readerDraws.filter((draw) => draw.startsWith('m1 ') && draw !== 'm1 m0,m1')).toEqual([]); + }); + it('shows the message alone with "Show emails as individual messages"', async () => { useSettingsStore.setState({ disableThreading: true } as never); render(); diff --git a/components/mail/__tests__/mail-app-source-routing.test.tsx b/components/mail/__tests__/mail-app-source-routing.test.tsx index add5d3d12..fdb670d02 100644 --- a/components/mail/__tests__/mail-app-source-routing.test.tsx +++ b/components/mail/__tests__/mail-app-source-routing.test.tsx @@ -272,7 +272,8 @@ describe('shared message deep link from the personal Inbox (#1491)', () => { }); const mounted = render(); try { - await waitFor(() => expect(fetchContent).toHaveBeenCalledWith(client, 'eaaaaab', 'ce')); + // Fetched without opening: it opens with its conversation, in the reading pane. + await waitFor(() => expect(fetchContent).toHaveBeenCalledWith(client, 'eaaaaab', 'ce', { select: false })); await waitFor(() => expect(useEmailStore.getState().selectedMailbox).toBe('ce:a')); expect(fetchEmails).toHaveBeenCalledWith(client, 'ce:a', { background: true }); expect(useEmailStore.getState().selectedEmail?.subject).toBe('GRP one'); diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index f1192cee7..a2b1548fc 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -396,6 +396,13 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { threadId: string; emailId: string; startedAt: number; promise: Promise; } | null>(null); const mvConversationOpenSeqRef = useRef(0); + // The open that put a message on screen together with its conversation (or + // with none, a message alone in its thread). The effect below then has + // nothing to fetch, and resetting and setting it again would draw the whole + // mail view twice more for each message opened. + const mvConversationSettledRef = useRef<{ + emailId: string; threadId: string; client: EmailBlobContext['client']; accountId?: string; + } | null>(null); // compose-p2-6: a reply to the open message saved as a draft. The reader // shows it under the message, and Reply opens it again. The conversation // fetch notes the thread's drafts; a scan reads them fresh after a composer @@ -2299,11 +2306,30 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { } setLoadingEmail(true); try { - const email = await fetchEmailContent(activeClient, link.id, link.ownerAccountId); + // With the conversation in the reader, the message waits for it, as on + // a click: shown alone first, it would move down once it came. + const withConversation = readerConversationView && !cursorOnly; + const email = withConversation + ? await fetchEmailContent(activeClient, link.id, link.ownerAccountId, { select: false }) + : await fetchEmailContent(activeClient, link.id, link.ownerAccountId); if (!email) { toast.error(t('deep_link.message_not_found')); return; } + if (withConversation) { + const source = resolveEmailBlobContext(email, activeClient); + const conversation = await Promise.race([ + loadMvConversation(email, activeClient), + new Promise((resolve) => setTimeout(() => resolve(undefined), MV_CONVERSATION_WAIT_MS)), + ]); + if (conversation !== undefined) { + setMvConversationResult(conversation ? { emails: conversation, source, threadId: email.threadId } : null); + mvConversationSettledRef.current = { + emailId: email.id, threadId: email.threadId, client: source.client, accountId: source.accountId, + }; + } + useEmailStore.setState({ selectedEmail: email }); + } await showInOwningFolder(activeClient, email); if (link.fullscreen && !isMobile) { // The owning-folder branch above may have skipped selecting (the @@ -4990,11 +5016,19 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { useEffect(() => { ++mvConversationOpenSeqRef.current; + const settled = mvConversationSettledRef.current; + mvConversationSettledRef.current = null; if (!mvConversationThreadId || !client || !mvConversationRef) { setMvConversationResult(null); setMvConversationLoading(null); return; } + if (settled && settled.emailId === mvConversationRef.id && settled.threadId === mvConversationThreadId + && settled.client === mvBlobSource.client && settled.accountId === mvBlobSource.accountId) { + // The open already has the conversation on screen with the message. + setMvConversationLoading(null); + return () => { ++mvConversationOpenSeqRef.current; }; + } let cancelled = false; setMvConversationResult(null); setMvConversationLoading(mvConversationThreadId); @@ -5442,9 +5476,14 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { new Promise((resolve) => setTimeout(() => resolve(undefined), MV_CONVERSATION_WAIT_MS)), ]); if (openSeq !== emailOpenSeqRef.current) return 'opened'; - if (conversation !== undefined) setMvConversationResult(conversation ? { - emails: conversation, source, threadId: fullEmail.threadId, - } : null); + if (conversation !== undefined) { + setMvConversationResult(conversation ? { + emails: conversation, source, threadId: fullEmail.threadId, + } : null); + mvConversationSettledRef.current = { + emailId: fullEmail.id, threadId: fullEmail.threadId, client: source.client, accountId: source.accountId, + }; + } } // One write: the message, and the end of its loading state. finishOpenEmail(fullEmail); diff --git a/stores/email-store.ts b/stores/email-store.ts index 429d1dcdc..69720d6a4 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -467,8 +467,11 @@ interface EmailStore { // times; later calls are no-ops while a prior one is in flight. prefetchInitialData: (client: IJMAPClient) => Promise; loadMoreEmails: (client: IJMAPClient) => Promise; - /** `ownerAccountId`: the JMAP account a link names for the message (#1351). */ - fetchEmailContent: (client: IJMAPClient, emailId: string, ownerAccountId?: string) => Promise; + /** + * `ownerAccountId`: the JMAP account a link names for the message (#1351). + * `select: false`: hand the message back without opening it. + */ + fetchEmailContent: (client: IJMAPClient, emailId: string, ownerAccountId?: string, options?: { select?: boolean }) => Promise; fetchQuota: (client: IJMAPClient) => Promise; /** * `options.outbox`: a send from the composer or quick reply, which offline @@ -4349,7 +4352,7 @@ export const useEmailStore = create((set, get) => ({ } }, - fetchEmailContent: async (client, emailId, ownerAccountId) => { + fetchEmailContent: async (client, emailId, ownerAccountId, options) => { try { // Route to the owning account. In aggregate views (All Mail, unified, // cross-account) the selected mailbox is virtual, so derive the client + @@ -4382,7 +4385,7 @@ export const useEmailStore = create((set, get) => ({ annotatedEmail.accountId = listEmail?.accountId; annotatedEmail.accountLabel = listEmail?.accountLabel; annotatedEmail.sourceFolder = listEmail?.sourceFolder; - set({ selectedEmail: annotatedEmail }); + if (options?.select !== false) set({ selectedEmail: annotatedEmail }); return annotatedEmail; } return email;