fix: delete only the clicked mail when another account has a message with the same id #857

This commit is contained in:
Linus Rath
2026-10-17 19:18:00 +02:00
parent f8ef54dbea
commit 7d882be734
3 changed files with 157 additions and 15 deletions
+3 -3
View File
@@ -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');
@@ -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>): 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<string, ReturnType<typeof vi.fn>>;
// 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<typeof makeClient>;
let clientB: ReturnType<typeof makeClient>;
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']);
});
});
+48 -12
View File
@@ -307,9 +307,11 @@ interface EmailStore {
fetchQuota: (client: IJMAPClient) => Promise<void>;
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<SendEmailResult>;
sendRawEmail: (client: IJMAPClient, rawMimeBlob: Blob, identityId: string, delayedUntil?: string, envelopeRecipients?: string[]) => Promise<SendEmailResult>;
deleteEmail: (client: IJMAPClient, emailId: string, forceDelete?: boolean) => Promise<boolean>;
/** `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<boolean>;
markAsRead: (client: IJMAPClient, emailId: string, read: boolean) => Promise<boolean>;
moveToMailbox: (client: IJMAPClient, emailId: string, mailboxId: string) => Promise<boolean>;
/** `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<boolean>;
/**
* 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<string, ScheduledSubmissionMetadata>
@@ -3080,10 +3114,11 @@ export const useEmailStore = create<EmailStore>((set, get) => ({
}
},
deleteEmail: async (client, emailId, forceDelete): Promise<boolean> => {
deleteEmail: async (client, emailId, forceDelete, emailKey): Promise<boolean> => {
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<EmailStore>((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<EmailStore>((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<EmailStore>((set, get) => ({
);
},
moveToMailbox: async (client, emailId, destinationMailboxId): Promise<boolean> => {
moveToMailbox: async (client, emailId, destinationMailboxId, emailKey): Promise<boolean> => {
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<EmailStore>((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,
};
});