From 7d882be734e998fc710d8033ba4a2bc215a89132 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 17 Oct 2026 01:37:44 +0200 Subject: [PATCH] fix: delete only the clicked mail when another account has a message with the same id #857 --- components/mail/mail-app.tsx | 6 +- .../__tests__/email-single-namesakes.test.ts | 106 ++++++++++++++++++ stores/email-store.ts | 60 ++++++++-- 3 files changed, 157 insertions(+), 15 deletions(-) create mode 100644 stores/__tests__/email-single-namesakes.test.ts diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 767bef433..170d9c29a 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -61,7 +61,7 @@ import { debug } from "@/lib/debug"; import { playNotificationSound } from "@/lib/notification-sound"; import { cn, getMailboxPath } from "@/lib/utils"; import { localizeMailboxName } from "@/lib/mailbox-label"; -import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY, groupEmailsByThread, threadKeyFor } from "@/lib/thread-utils"; +import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY, emailKeyFor, groupEmailsByThread, threadKeyFor } from "@/lib/thread-utils"; import { resolveThreadRoute } from "@/lib/thread-routing"; import { ErrorBoundary, @@ -2692,7 +2692,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { try { // false: a plugin stopped it and said why, so no "deleted" here. - if (await deleteEmail(client, emailToDelete.id, true)) toast.success(t('notifications.email_deleted')); + if (await deleteEmail(client, emailToDelete.id, true, emailKeyFor(emailToDelete))) toast.success(t('notifications.email_deleted')); } catch (error) { console.error("Failed to permanently delete email:", error); toast.error(t('notifications.error_deleting'), error instanceof Error ? error.message : undefined); @@ -2714,7 +2714,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { }); if (trashMailbox) { try { - if (await moveToMailbox(client, emailToDelete.id, trashMailbox.id)) toast.success(t('notifications.email_deleted')); + if (await moveToMailbox(client, emailToDelete.id, trashMailbox.id, emailKeyFor(emailToDelete))) toast.success(t('notifications.email_deleted')); } catch (error) { console.error("Failed to move email to trash:", error); toast.error(error instanceof Error ? error.message : 'Failed to move email to trash'); diff --git a/stores/__tests__/email-single-namesakes.test.ts b/stores/__tests__/email-single-namesakes.test.ts new file mode 100644 index 000000000..bfc466d82 --- /dev/null +++ b/stores/__tests__/email-single-namesakes.test.ts @@ -0,0 +1,106 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { useEmailStore } from '../email-store'; +import { useAuthStore } from '../auth-store'; +import { useSettingsStore } from '../settings-store'; +import { emailKeyFor } from '@/lib/thread-utils'; +import type { Email, Mailbox } from '@/lib/jmap/types'; +import type { IJMAPClient } from '@/lib/jmap/client-interface'; + +/** + * Deleting one row of the unified view while another account holds a message + * with the same id (Stalwart hands out the same ids, and the same trash id, + * in every account). The action must reach the row that was clicked and leave + * its namesake listed and untouched (#857). + */ +const twin = (account: string, receivedAt: string): Email => ({ + id: 'eiaaaabc', threadId: `t-${account}`, blobId: `blob-${account}`, + mailboxIds: { 'a': true }, keywords: { $seen: true }, + from: [{ email: `someone@${account}` }], to: [], subject: account, + receivedAt, size: 42, preview: '', hasAttachment: false, + sourceClientAccountId: account, sourceAccountId: account, +} as unknown as Email); + +const mailbox = (overrides: Partial): Mailbox => ({ + id: 'a', name: 'Inbox', sortOrder: 0, + totalEmails: 1, unreadEmails: 0, totalThreads: 1, unreadThreads: 0, + myRights: { + mayReadItems: true, mayAddItems: true, mayRemoveItems: true, maySetSeen: true, + maySetKeywords: true, mayCreateChild: true, mayRename: true, mayDelete: true, maySubmit: true, + }, + isSubscribed: true, isShared: false, + ...overrides, +} as Mailbox); + +const makeClient = () => ({ + moveEmail: vi.fn().mockResolvedValue(undefined), + moveToTrash: vi.fn().mockResolvedValue(undefined), + deleteEmail: vi.fn().mockResolvedValue(undefined), + getMailboxes: vi.fn().mockResolvedValue([]), +}) as unknown as IJMAPClient & Record>; + +// The list is newest first: account A's namesake sits right above the row +// that gets deleted. +const above = twin('account-a', '2026-10-16T09:00:00Z'); +const clicked = twin('account-b', '2026-10-16T08:00:00Z'); +let clientA: ReturnType; +let clientB: ReturnType; + +const accountMailboxes = (account: string) => [ + mailbox({ id: 'a', role: 'inbox', accountId: account }), + mailbox({ id: 'b', name: 'Trash', role: 'trash', accountId: account }), +]; + +beforeEach(() => { + clientA = makeClient(); + clientB = makeClient(); + useAuthStore.setState({ + activeAccountId: 'account-a', + client: clientA, + getClientForAccount: (id: string) => + (id === 'account-a' ? clientA : id === 'account-b' ? clientB : undefined) as never, + } as never); + useSettingsStore.setState({ deleteAction: 'trash' } as never); + useEmailStore.setState({ + isUnifiedView: true, + unifiedRole: 'inbox', + selectedMailbox: '', + selectedKeyword: null, + emails: [above, clicked], + selectedEmail: clicked, + selectedEmailKeys: new Set(), + mailboxes: [], + accountMailboxes: { + 'account-a': accountMailboxes('account-a'), + 'account-b': accountMailboxes('account-b'), + }, + error: null, + } as never); +}); + +describe('single-email actions with a namesake in another account', () => { + it('moves only the clicked row to trash and keeps the row above it', async () => { + await useEmailStore.getState().moveToMailbox(clientA, clicked.id, 'b', emailKeyFor(clicked)); + + expect(clientB.moveEmail).toHaveBeenCalledWith('eiaaaabc', 'b', 'account-b'); + expect(clientA.moveEmail).not.toHaveBeenCalled(); + const state = useEmailStore.getState(); + expect(state.emails.map(e => e.sourceAccountId)).toEqual(['account-a']); + expect(state.selectedEmail?.sourceAccountId).toBe('account-a'); + }); + + it('permanently deletes only the clicked row', async () => { + await useEmailStore.getState().deleteEmail(clientA, clicked.id, true, emailKeyFor(clicked)); + + expect(clientB.deleteEmail).toHaveBeenCalledWith('eiaaaabc', 'account-b'); + expect(clientA.deleteEmail).not.toHaveBeenCalled(); + expect(useEmailStore.getState().emails.map(e => e.sourceAccountId)).toEqual(['account-a']); + }); + + it('moves only the clicked row with the trash delete action', async () => { + await useEmailStore.getState().deleteEmail(clientA, clicked.id, false, emailKeyFor(clicked)); + + expect(clientB.moveToTrash).toHaveBeenCalledWith('eiaaaabc', 'b', 'account-b', false); + expect(clientA.moveToTrash).not.toHaveBeenCalled(); + expect(useEmailStore.getState().emails.map(e => e.sourceAccountId)).toEqual(['account-a']); + }); +}); diff --git a/stores/email-store.ts b/stores/email-store.ts index aecbea409..177fe04a7 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -307,9 +307,11 @@ interface EmailStore { fetchQuota: (client: IJMAPClient) => Promise; sendEmail: (client: IJMAPClient, to: string[], subject: string, body: string, cc?: string[], bcc?: string[], identityId?: string, fromEmail?: string, draftId?: string, fromName?: string, htmlBody?: string, attachments?: Array<{ blobId: string; name: string; type: string; size: number; disposition?: 'attachment' | 'inline'; cid?: string }>, inReplyTo?: string[], references?: string[], delayedUntil?: string, envelopeMailFrom?: string, options?: { requestReadReceipt?: boolean; requestDsn?: boolean; requireTls?: boolean; localAccountId?: string }) => Promise; sendRawEmail: (client: IJMAPClient, rawMimeBlob: Blob, identityId: string, delayedUntil?: string, envelopeRecipients?: string[]) => Promise; - deleteEmail: (client: IJMAPClient, emailId: string, forceDelete?: boolean) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + deleteEmail: (client: IJMAPClient, emailId: string, forceDelete?: boolean, emailKey?: string) => Promise; markAsRead: (client: IJMAPClient, emailId: string, read: boolean) => Promise; - moveToMailbox: (client: IJMAPClient, emailId: string, mailboxId: string) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + moveToMailbox: (client: IJMAPClient, emailId: string, mailboxId: string, emailKey?: string) => Promise; /** * Move a single email, routing across the account boundary when the * destination folder is owned by a different JMAP account (a delegated/shared @@ -502,6 +504,38 @@ function getNextSelectedEmail(state: { emails: Email[]; selectedEmail: Email | n return getNextSelectedEmailAfterRemoval(state, new Set([removedEmailId])); } +/** + * Matches the list row a single-email action is about. Email ids repeat across + * accounts, so in a merged view (unified inbox, tags) the bare id also matches + * the other account's namesake: deleting one row moved or hid the other as + * well (#857). With the row's `emailKeyFor` key only that row matches. + */ +function singleEmailMatcher(emailId: string, emailKey?: string): (email: Email) => boolean { + return emailKey ? (email) => emailKeyFor(email) === emailKey : (email) => email.id === emailId; +} + +/** getNextSelectedEmail for the one row `isRemoved` matches. */ +function getNextSelectedEmailAfterRemovingRow( + state: { emails: Email[]; selectedEmail: Email | null }, + isRemoved: (email: Email) => boolean, +): Email | null { + const selected = state.selectedEmail; + if (!selected || !isRemoved(selected)) return selected; + + const selectedKey = emailKeyFor(selected); + let idx = state.emails.findIndex(e => emailKeyFor(e) === selectedKey); + if (idx === -1) idx = state.emails.findIndex(e => e.id === selected.id); + if (idx === -1) return null; + + for (let nextIndex = idx + 1; nextIndex < state.emails.length; nextIndex++) { + if (!isRemoved(state.emails[nextIndex])) return state.emails[nextIndex]; + } + for (let prevIndex = idx - 1; prevIndex >= 0; prevIndex--) { + if (!isRemoved(state.emails[prevIndex])) return state.emails[prevIndex]; + } + return null; +} + function annotateScheduledEmails( emails: Email[], scheduledSubmissionByEmailId: Map @@ -3080,10 +3114,11 @@ export const useEmailStore = create((set, get) => ({ } }, - deleteEmail: async (client, emailId, forceDelete): Promise => { + deleteEmail: async (client, emailId, forceDelete, emailKey): Promise => { try { // Get the email to check if it's unread and which mailboxes it belongs to - const email = get().emails.find(e => e.id === emailId); + const isTarget = singleEmailMatcher(emailId, emailKey); + const email = get().emails.find(isTarget); if (!email) return false; const isUnread = !email.keywords?.$seen; @@ -3157,8 +3192,8 @@ export const useEmailStore = create((set, get) => ({ : {}; return { - emails: state.emails.filter(e => e.id !== emailId), - selectedEmail: getNextSelectedEmail(state, emailId), + emails: state.emails.filter(e => !isTarget(e)), + selectedEmail: getNextSelectedEmailAfterRemovingRow(state, isTarget), ...mailboxPatch, }; }); @@ -3191,8 +3226,8 @@ export const useEmailStore = create((set, get) => ({ : {}; return { - emails: state.emails.filter(e => e.id !== emailId), - selectedEmail: getNextSelectedEmail(state, emailId), + emails: state.emails.filter(e => !isTarget(e)), + selectedEmail: getNextSelectedEmailAfterRemovingRow(state, isTarget), ...mailboxPatch, }; }); @@ -3380,9 +3415,10 @@ export const useEmailStore = create((set, get) => ({ ); }, - moveToMailbox: async (client, emailId, destinationMailboxId): Promise => { + moveToMailbox: async (client, emailId, destinationMailboxId, emailKey): Promise => { try { - const email = get().emails.find(e => e.id === emailId); + const isTarget = singleEmailMatcher(emailId, emailKey); + const email = get().emails.find(isTarget); if (!email) return false; const isUnread = !email.keywords?.$seen; @@ -3426,8 +3462,8 @@ export const useEmailStore = create((set, get) => ({ }); return { - emails: state.emails.filter(e => e.id !== emailId), - selectedEmail: getNextSelectedEmail(state, emailId), + emails: state.emails.filter(e => !isTarget(e)), + selectedEmail: getNextSelectedEmailAfterRemovingRow(state, isTarget), ...mailboxPatch, }; });