From bfe98b49a093d498378aae2fd161e717db4859d4 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 18 Oct 2026 17:54:18 +0200 Subject: [PATCH] fix: act on the clicked row when another account has a message with the same id (GHSA-8x23-qw3w-mhrx) --- components/email/email-list.tsx | 14 +- components/email/thread-list-item.tsx | 6 +- components/mail/mail-app.tsx | 71 +++++----- components/pro/pro-email-tab-body.tsx | 8 +- .../__tests__/email-single-namesakes.test.ts | 111 +++++++++++++++ .../email-store-cross-account-move.test.ts | 2 +- stores/email-store.ts | 132 ++++++++++-------- 7 files changed, 239 insertions(+), 105 deletions(-) diff --git a/components/email/email-list.tsx b/components/email/email-list.tsx index b98fa93a8..09b3f6705 100644 --- a/components/email/email-list.tsx +++ b/components/email/email-list.tsx @@ -64,8 +64,9 @@ interface EmailListProps { onTogglePinned?: (email: Email) => void; onDelete?: (email: Email) => void; onArchive?: (email: Email) => void; - onSetTag?: (emailId: string, tagId: string | null) => void; - onMoveToMailbox?: (emailId: string, mailboxId: string) => void; + /** `emailKey` (emailKeyFor of the row) tells namesakes in other accounts apart. */ + onSetTag?: (emailId: string, tagId: string | null, emailKey?: string) => void; + onMoveToMailbox?: (emailId: string, mailboxId: string, emailKey?: string) => void; onMarkAsSpam?: (email: Email) => void; onUndoSpam?: (email: Email) => void; onOpenAttachment?: (email: Email, attachment: Attachment) => void; @@ -252,8 +253,11 @@ export function EmailList({ * the message it was handed when it opened, but tags can be applied from * inside it without dismissing it, so what it draws has to keep up. */ + // The live copy of the row the menu was opened on, by its key: in a merged + // view the bare id is also another account's message, and every action of + // the menu would go to that namesake instead. const contextMenuEmail = contextMenu.data - ? emails.find((email) => email.id === contextMenu.data!.id) ?? contextMenu.data + ? emails.find((email) => emailKeyFor(email) === emailKeyFor(contextMenu.data!)) ?? contextMenu.data : null; /** * Whose sender the menu's Rules entry uses: every selected message, or the @@ -1034,8 +1038,8 @@ export function EmailList({ onTogglePinned={onTogglePinned ? () => onTogglePinned(contextMenuEmail!) : undefined} onDelete={() => onDelete?.(contextMenuEmail!)} onArchive={() => onArchive?.(contextMenuEmail!)} - onSetTag={(color) => onSetTag?.(contextMenuEmail!.id, color)} - onMoveToMailbox={(mailboxId) => onMoveToMailbox?.(contextMenuEmail!.id, mailboxId)} + onSetTag={(color) => onSetTag?.(contextMenuEmail!.id, color, emailKeyFor(contextMenuEmail!))} + onMoveToMailbox={(mailboxId) => onMoveToMailbox?.(contextMenuEmail!.id, mailboxId, emailKeyFor(contextMenuEmail!))} onRemoveFromFolder={() => void removeFromFolder([contextMenuEmail!])} onMarkAsSpam={() => onMarkAsSpam?.(contextMenuEmail!)} onUndoSpam={() => onUndoSpam?.(contextMenuEmail!)} diff --git a/components/email/thread-list-item.tsx b/components/email/thread-list-item.tsx index 3944c1f5e..2ccafb6b6 100644 --- a/components/email/thread-list-item.tsx +++ b/components/email/thread-list-item.tsx @@ -304,7 +304,7 @@ interface ThreadListItemProps { onMarkAsRead?: (email: Email, read: boolean) => void; onDelete?: (email: Email) => void; onArchive?: (email: Email) => void; - onSetTag?: (emailId: string, tagId: string | null) => void; + onSetTag?: (emailId: string, tagId: string | null, emailKey?: string) => void; onMarkAsSpam?: (email: Email) => void; onUndoSpam?: (email: Email) => void; onOpenAttachment?: (email: Email, attachment: Attachment) => void; @@ -1030,7 +1030,7 @@ export const ThreadListItem = React.forwardRef onMarkAsRead(latestEmail, read) : undefined} onDelete={onDelete ? () => onDelete(latestEmail) : undefined} onArchive={onArchive ? () => onArchive(latestEmail) : undefined} - onSetTag={onSetTag ? (color) => onSetTag(latestEmail.id, color) : undefined} + onSetTag={onSetTag ? (color) => onSetTag(latestEmail.id, color, emailKeyFor(latestEmail)) : undefined} onMarkAsSpam={onMarkAsSpam ? () => onMarkAsSpam(latestEmail) : undefined} onUndoSpam={onUndoSpam ? () => onUndoSpam(latestEmail) : undefined} onOpenAttachment={onOpenAttachment ? (a) => onOpenAttachment(latestEmail, a) : undefined} @@ -1537,7 +1537,7 @@ export const ThreadListItem = React.forwardRef onMarkAsRead(latestEmail, read) : undefined} onDelete={onDelete ? () => onDelete(latestEmail) : undefined} onArchive={onArchive ? () => onArchive(latestEmail) : undefined} - onSetTag={onSetTag ? (color) => onSetTag(latestEmail.id, color) : undefined} + onSetTag={onSetTag ? (color) => onSetTag(latestEmail.id, color, emailKeyFor(latestEmail)) : undefined} onMarkAsSpam={onMarkAsSpam ? () => onMarkAsSpam(latestEmail) : undefined} onUndoSpam={onUndoSpam ? () => onUndoSpam(latestEmail) : undefined} isInJunk={currentMailboxRole === 'junk'} diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 2bbc64707..0ac5dc1a7 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -838,7 +838,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const keyboardHandlers = useMemo(() => ({ onNextEmail: () => { if (activeEmails.length === 0) return; - const currentIndex = selectedEmail ? activeEmails.findIndex(e => e.id === selectedEmail.id) : -1; + const currentIndex = selectedEmail ? activeEmails.findIndex(e => emailKeyFor(e) === emailKeyFor(selectedEmail)) : -1; const nextIndex = currentIndex < activeEmails.length - 1 ? currentIndex + 1 : currentIndex; if (nextIndex >= 0 && nextIndex < activeEmails.length) { void handleEmailSelect(activeEmails[nextIndex]); @@ -847,7 +847,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { }, onPreviousEmail: () => { if (activeEmails.length === 0) return; - const currentIndex = selectedEmail ? activeEmails.findIndex(e => e.id === selectedEmail.id) : activeEmails.length; + const currentIndex = selectedEmail ? activeEmails.findIndex(e => emailKeyFor(e) === emailKeyFor(selectedEmail)) : activeEmails.length; const prevIndex = currentIndex > 0 ? currentIndex - 1 : 0; if (prevIndex >= 0 && prevIndex < activeEmails.length) { void handleEmailSelect(activeEmails[prevIndex]); @@ -946,7 +946,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (shortcutsTargetSelection) { await batchMarkAsRead(client, false); } else if (selectedEmail) { - await markAsRead(client, selectedEmail.id, false); + await markAsRead(client, selectedEmail.id, false, emailKeyFor(selectedEmail)); } }, onMarkAsRead: async () => { @@ -955,7 +955,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (shortcutsTargetSelection) { await batchMarkAsRead(client, true); } else if (selectedEmail) { - await markAsRead(client, selectedEmail.id, true); + await markAsRead(client, selectedEmail.id, true, emailKeyFor(selectedEmail)); } }, onToggleSpam: async () => { @@ -1953,13 +1953,13 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { } else if (markAsReadDelay === 0) { // Mark as read instantly debug.log('email', '[Mark as Read] Instant mode - marking as read now'); - markAsRead(client, selectedEmail.id, true); + markAsRead(client, selectedEmail.id, true, emailKeyFor(selectedEmail)); } else { // Mark as read after delay debug.log('email', '[Mark as Read] Delayed mode - will mark as read in', markAsReadDelay, 'ms'); markAsReadTimeoutRef.current = setTimeout(() => { debug.log('email', '[Mark as Read] Timeout fired - marking as read now'); - markAsRead(client, selectedEmail.id, true); + markAsRead(client, selectedEmail.id, true, emailKeyFor(selectedEmail)); markAsReadTimeoutRef.current = null; }, markAsReadDelay); } @@ -2798,7 +2798,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { let archived: boolean; if (archiveMode === 'single') { - archived = await moveThreadToMailbox(client, emailToArchive.id, archiveMailbox.id); + archived = await moveThreadToMailbox(client, emailToArchive.id, archiveMailbox.id, emailKeyFor(emailToArchive)); } else { const emailDate = new Date(emailToArchive.receivedAt); const year = emailDate.getFullYear().toString(); @@ -2815,7 +2815,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { } if (archiveMode === 'year') { - archived = await moveThreadToMailbox(client, emailToArchive.id, yearMailbox.id); + archived = await moveThreadToMailbox(client, emailToArchive.id, yearMailbox.id, emailKeyFor(emailToArchive)); } else { const yearId = yearMailbox.originalId || yearMailbox.id; const afterYear = readMailboxes(); @@ -2827,7 +2827,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (!monthMailbox) return; // a plugin refused the folder await refreshMailboxes(); } - archived = await moveThreadToMailbox(client, emailToArchive.id, monthMailbox.id); + archived = await moveThreadToMailbox(client, emailToArchive.id, monthMailbox.id, emailKeyFor(emailToArchive)); } } if (!archived) return; @@ -2855,13 +2855,13 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const folderToLeave = useEmailStore(removalFolder); const openEmailCanLeaveFolder = !!selectedEmail && canRemoveFromFolder(selectedEmail, folderToLeave); - const handleMoveToMailbox = async (emailId: string, mailboxId: string) => { + const handleMoveToMailbox = async (emailId: string, mailboxId: string, emailKey?: string) => { if (!client) return; const destinationList = [mailboxes, ...Object.values(accountMailboxes)] .find(list => list.some(m => m.id === mailboxId)); const destination = destinationList?.find(m => m.id === mailboxId); try { - if (!(await moveToMailboxCrossAware(client, emailId, mailboxId))) return; + if (!(await moveToMailboxCrossAware(client, emailId, mailboxId, emailKey))) return; toast.success( t('notifications.email_moved'), destination && destinationList @@ -2878,7 +2878,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (!client || !selectedEmail) return; try { - await toggleStar(client, selectedEmail.id); + await toggleStar(client, selectedEmail.id, emailKeyFor(selectedEmail)); } catch (error) { console.error("Failed to toggle star:", error); toast.error(t('notifications.error_updating')); @@ -2891,14 +2891,14 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { const emailId = emailToMark.id; try { - if (!(await markAsSpam(client, emailId))) return; + if (!(await markAsSpam(client, emailId, emailKeyFor(emailToMark)))) return; toast.success(t('email_viewer.spam.toast_success'), { action: { label: t('email_viewer.spam.toast_undo'), onClick: async () => { try { - if (await undoSpam(client, emailId)) toast.success(t('notifications.email_moved')); + if (await undoSpam(client, emailId, emailKeyFor(emailToMark))) toast.success(t('notifications.email_moved')); } catch (_error) { console.error("Failed to undo spam:", _error); toast.error(t('email_viewer.spam.error')); @@ -2917,7 +2917,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (!client || !emailToRestore) return; try { - if (!(await undoSpam(client, emailToRestore.id))) return; + if (!(await undoSpam(client, emailToRestore.id, emailKeyFor(emailToRestore)))) return; toast.success(t('email_viewer.spam.toast_not_spam_success')); } catch (_error) { @@ -2930,7 +2930,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (!client) return; try { - const email = emails.find(e => e.id === emailToPin.id) ?? emailToPin; + const email = emails.find(e => emailKeyFor(e) === emailKeyFor(emailToPin)) ?? emailToPin; const isPinned = email.keywords?.['$pinned'] === true; // Same routing as tags: the write goes to the email's own account, and @@ -2940,7 +2940,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { // would replace a tag-filtered list (refreshCurrentMailbox fetches by // folder only). (#281) // Only $pinned changes; the row's other keywords may be stale. - await patchEmailKeywords(client, email.id, { $pinned: !isPinned }); + await patchEmailKeywords(client, email.id, { $pinned: !isPinned }, emailKeyFor(email)); if (!isUnifiedView && !useEmailStore.getState().selectedKeyword) { void refreshCurrentMailbox(client); } @@ -2950,12 +2950,13 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { } }; - const handleSetTag = async (emailId: string, tagId: string | null) => { + const handleSetTag = async (emailId: string, tagId: string | null, emailKey?: string) => { if (!client) return; try { - // Remove any existing tag keywords - const email = emails.find(e => e.id === emailId); + // Remove any existing tag keywords. By the row's key when given: the bare + // id can also be another account's message in a merged view. + const email = emailKey ? emails.find(e => emailKeyFor(e) === emailKey) : emails.find(e => e.id === emailId); if (!email) return; // Only the tag keywords change: writing the whole set from this row @@ -2991,7 +2992,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { // call site covered only the unified view, so a tag set on a directly // selected shared folder was written to the reaching account and silently // dropped by the server. (#281) - await patchEmailKeywords(client, emailId, changes); + await patchEmailKeywords(client, emailId, changes, emailKeyFor(email)); // Refresh tag counts fetchTagCounts(client); @@ -3971,9 +3972,9 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { if (!client || target.keywords?.$seen) return; try { if (useEmailStore.getState().emails.some((e) => e.id === target.id)) { - await markAsRead(client, target.id, true); + await markAsRead(client, target.id, true, emailKeyFor(target)); // Still unread: a plugin stopped it, so the copy here stays unread too. - if (!useEmailStore.getState().emails.find((e) => e.id === target.id)?.keywords?.$seen) return; + if (!useEmailStore.getState().emails.find((e) => emailKeyFor(e) === emailKeyFor(target))?.keywords?.$seen) return; } else { // Not in the store's list, so the store action can't ask plugins. if (emailHooks.onBeforeEmailReadStateChange.size > 0 @@ -4212,7 +4213,7 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { }; // Navigate to next/previous email in the list - const selectedEmailIndex = selectedEmail ? activeEmails.findIndex(e => e.id === selectedEmail.id) : -1; + const selectedEmailIndex = selectedEmail ? activeEmails.findIndex(e => emailKeyFor(e) === emailKeyFor(selectedEmail)) : -1; const handleNavigateNext = selectedEmailIndex >= 0 && selectedEmailIndex < activeEmails.length - 1 ? () => handleEmailSelect(activeEmails[selectedEmailIndex + 1]) @@ -4709,9 +4710,8 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { // The open message when it is in this list, else the // first row - as an email, so the selection is keyed by // its owning account and not by a colliding id. - const target = (selectedEmail && activeEmails.some((e) => e.id === selectedEmail.id) - ? activeEmails.find((e) => e.id === selectedEmail.id) - : activeEmails[0])!; + const openKey = selectedEmail ? emailKeyFor(selectedEmail) : null; + const target = activeEmails.find((e) => emailKeyFor(e) === openKey) ?? activeEmails[0]; toggleEmailSelection(target); } }} @@ -5058,12 +5058,12 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { }} onMarkAsRead={async (email, read) => { if (client) { - await markAsRead(client, email.id, read); + await markAsRead(client, email.id, read, emailKeyFor(email)); } }} onToggleStar={async (email) => { if (client) { - await toggleStar(client, email.id); + await toggleStar(client, email.id, emailKeyFor(email)); } }} onTogglePinned={async (email) => { @@ -5075,8 +5075,8 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { onArchive={archiveActionEnabled ? async (email) => { await handleArchive(email); } : undefined} - onSetTag={(emailId, color) => { - handleSetTag(emailId, color); + onSetTag={(emailId, color, emailKey) => { + handleSetTag(emailId, color, emailKey); }} onMoveToMailbox={handleMoveToMailbox} onMarkAsSpam={async (email) => { @@ -5212,7 +5212,8 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { onDownloadAttachment={handleDownloadAttachment} onMarkAsRead={async (emailId, read) => { if (client) { - await markAsRead(client, emailId, read); + const target = conversationEmails.find((e) => e.id === emailId); + await markAsRead(client, emailId, read, target ? emailKeyFor(target) : undefined); } }} mailboxRole={mailboxes.find(m => m.id === selectedMailbox)?.role ?? (isUnifiedView ? (unifiedRole ?? undefined) : undefined)} @@ -5246,8 +5247,10 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { onUndoSpam={() => handleUndoSpam()} onMarkAsRead={async (emailId, read) => { if (client) { - await markAsRead(client, emailId, read); - const nowRead = !!useEmailStore.getState().emails.find((e) => e.id === emailId)?.keywords?.$seen; + const emailKey = selectedEmail?.id === emailId ? emailKeyFor(selectedEmail) : undefined; + await markAsRead(client, emailId, read, emailKey); + const nowRead = !!useEmailStore.getState().emails + .find((e) => (emailKey ? emailKeyFor(e) === emailKey : e.id === emailId))?.keywords?.$seen; if (nowRead !== read) return; // a plugin stopped it // Marking the open message unread returns to the list // (Gmail-style, gated on returnToListAfterAction). Staying diff --git a/components/pro/pro-email-tab-body.tsx b/components/pro/pro-email-tab-body.tsx index 6a308fd6a..b51b900ac 100644 --- a/components/pro/pro-email-tab-body.tsx +++ b/components/pro/pro-email-tab-body.tsx @@ -19,7 +19,7 @@ import type { IJMAPClient } from "@/lib/jmap/client-interface"; import { buildReplySubject, buildForwardSubject } from "@/lib/subject-prefix"; import { getQuoteBodies } from "@/lib/email-composer-utils"; import { buildForwardAsAttachmentPayload } from "@/lib/forward-as-attachment"; -import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY } from "@/lib/thread-utils"; +import { KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY, emailKeyFor } from "@/lib/thread-utils"; import { cn } from "@/lib/utils"; import { getDraftTextBody } from '@/lib/email-body-selection'; import { messagePriorityFromHeaders } from '@/lib/message-priority'; @@ -252,7 +252,7 @@ export function ProEmailView({ emailId, client: clientOverride, accountId, onLoa const archiveMb = mailboxes.find((m) => m.role === 'archive'); if (!archiveMb) return; try { - if (!(await moveToMailbox(client, email.id, archiveMb.id))) return; + if (!(await moveToMailbox(client, email.id, archiveMb.id, emailKeyFor(email)))) return; toast.success(tNotifications('email_archived')); onClose(); } catch (err) { @@ -263,7 +263,7 @@ export function ProEmailView({ emailId, client: clientOverride, accountId, onLoa const handleToggleStar = useCallback(async () => { if (!client || !email) return; try { - if (!(await toggleStar(client, email.id))) return; + if (!(await toggleStar(client, email.id, emailKeyFor(email)))) return; // Reflect locally - the viewer re-reads from email-store's selectedEmail // shape only for the mail tab; here we update our local copy too. setEmail((prev) => prev ? { @@ -320,7 +320,7 @@ export function ProEmailView({ emailId, client: clientOverride, accountId, onLoa const handleMoveToMailbox = useCallback(async (mailboxId: string) => { if (!client || !email) return; try { - if (await moveToMailbox(client, email.id, mailboxId)) onClose(); + if (await moveToMailbox(client, email.id, mailboxId, emailKeyFor(email))) onClose(); } catch (err) { console.error('Move failed:', err); } diff --git a/stores/__tests__/email-single-namesakes.test.ts b/stores/__tests__/email-single-namesakes.test.ts index bfc466d82..306478aab 100644 --- a/stores/__tests__/email-single-namesakes.test.ts +++ b/stores/__tests__/email-single-namesakes.test.ts @@ -36,6 +36,11 @@ const makeClient = () => ({ moveToTrash: vi.fn().mockResolvedValue(undefined), deleteEmail: vi.fn().mockResolvedValue(undefined), getMailboxes: vi.fn().mockResolvedValue([]), + markAsRead: vi.fn().mockResolvedValue(undefined), + toggleStar: vi.fn().mockResolvedValue(undefined), + batchUpdateKeywords: vi.fn().mockResolvedValue(undefined), + markAsSpam: vi.fn().mockResolvedValue(undefined), + undoSpam: vi.fn().mockResolvedValue(undefined), }) as unknown as IJMAPClient & Record>; // The list is newest first: account A's namesake sits right above the row @@ -104,3 +109,109 @@ describe('single-email actions with a namesake in another account', () => { expect(useEmailStore.getState().emails.map(e => e.sourceAccountId)).toEqual(['account-a']); }); }); + +/** + * The other single-email actions resolved the row by its bare id as well, so in + * a merged view a hover star, a hover or menu read toggle, a tag or "spam" went + * to the FIRST row with that id - another account's message (measured against + * Stalwart 0.16: the hover star on one account's Sent message flagged another + * account's). With the row's key each reaches only the clicked row. + */ +describe('keyword and spam actions with a namesake in another account', () => { + beforeEach(() => { + useEmailStore.setState({ + emails: [above, { ...clicked, keywords: {} } as Email], + selectedEmail: null, + processingReadStatus: new Set(), + } as never); + }); + + it('stars only the clicked row', async () => { + await useEmailStore.getState().toggleStar(clientA, clicked.id, emailKeyFor(clicked)); + + expect(clientB.toggleStar).toHaveBeenCalledWith('eiaaaabc', true, 'account-b'); + expect(clientA.toggleStar).not.toHaveBeenCalled(); + const [a, b] = useEmailStore.getState().emails; + expect(a.keywords.$flagged).toBeFalsy(); + expect(b.keywords.$flagged).toBe(true); + }); + + it('marks only the clicked row read', async () => { + await useEmailStore.getState().markAsRead(clientA, clicked.id, true, emailKeyFor(clicked)); + + expect(clientB.markAsRead).toHaveBeenCalledWith('eiaaaabc', true, 'account-b'); + expect(clientA.markAsRead).not.toHaveBeenCalled(); + const [a, b] = useEmailStore.getState().emails; + expect(a.keywords.$seen).toBe(true); + expect(b.keywords.$seen).toBe(true); + }); + + it('marks only the clicked row unread and leaves the namesake read', async () => { + useEmailStore.setState({ emails: [above, clicked] } as never); + await useEmailStore.getState().markAsRead(clientA, clicked.id, false, emailKeyFor(clicked)); + + expect(clientB.markAsRead).toHaveBeenCalledWith('eiaaaabc', false, 'account-b'); + expect(clientA.markAsRead).not.toHaveBeenCalled(); + const [a, b] = useEmailStore.getState().emails; + expect(a.keywords.$seen).toBe(true); + expect(b.keywords.$seen).toBeFalsy(); + }); + + it('writes a keyword change only to the clicked row', async () => { + await useEmailStore.getState().patchEmailKeywords(clientA, clicked.id, { $label1: true }, emailKeyFor(clicked)); + + expect(clientB.batchUpdateKeywords).toHaveBeenCalledWith(['eiaaaabc'], { 'keywords/$label1': true }, 'account-b'); + expect(clientA.batchUpdateKeywords).not.toHaveBeenCalled(); + const [a, b] = useEmailStore.getState().emails; + expect(a.keywords.$label1).toBeUndefined(); + expect(b.keywords.$label1).toBe(true); + }); + + it('without a key keeps the previous single-account behaviour (bare id)', async () => { + useEmailStore.setState({ isUnifiedView: false, emails: [{ ...clicked, sourceClientAccountId: undefined, sourceAccountId: undefined } as Email] } as never); + await useEmailStore.getState().toggleStar(clientA, clicked.id); + + expect(clientA.toggleStar).toHaveBeenCalledWith('eiaaaabc', true, undefined); + }); +}); + +/** + * Spam and "not spam" took the row by its bare id too, the toast's undo cache + * was keyed by it, and the next open message was picked by it: with a namesake + * listed above, both rows dropped out of the selection walk. + */ +describe('spam actions with a namesake in another account', () => { + it('reports only the clicked row and opens the namesake next', async () => { + await useEmailStore.getState().markAsSpam(clientA, clicked.id, emailKeyFor(clicked)); + + expect(clientB.markAsSpam).toHaveBeenCalledWith('eiaaaabc', 'account-b', false); + expect(clientA.markAsSpam).not.toHaveBeenCalled(); + const state = useEmailStore.getState(); + expect(state.emails.map(e => e.sourceAccountId)).toEqual(['account-a']); + expect(state.selectedEmail?.sourceAccountId).toBe('account-a'); + }); + + it('moves only the clicked row out of junk with "not spam"', async () => { + // The list is refetched after the move; keep the local result in view. + useEmailStore.setState({ fetchUnifiedEmails: vi.fn() } as never); + await useEmailStore.getState().undoSpam(clientA, clicked.id, emailKeyFor(clicked)); + + expect(clientB.undoSpam).toHaveBeenCalledWith('eiaaaabc', 'a', 'account-b'); + expect(clientA.undoSpam).not.toHaveBeenCalled(); + const state = useEmailStore.getState(); + expect(state.emails.map(e => e.sourceAccountId)).toEqual(['account-a']); + expect(state.selectedEmail?.sourceAccountId).toBe('account-a'); + }); + + it('keeps one undo entry per row when both namesakes are reported', async () => { + useEmailStore.setState({ spamUndoCache: new Map() } as never); + await useEmailStore.getState().markAsSpam(clientA, clicked.id, emailKeyFor(clicked)); + useEmailStore.setState({ emails: [above] } as never); + await useEmailStore.getState().markAsSpam(clientA, above.id, emailKeyFor(above)); + + await useEmailStore.getState().undoSpam(clientA, clicked.id, emailKeyFor(clicked)); + + expect(clientB.undoSpam).toHaveBeenCalledWith('eiaaaabc', 'a', 'account-b'); + expect(clientA.undoSpam).not.toHaveBeenCalled(); + }); +}); diff --git a/stores/__tests__/email-store-cross-account-move.test.ts b/stores/__tests__/email-store-cross-account-move.test.ts index fcfdb9c26..c4fed054a 100644 --- a/stores/__tests__/email-store-cross-account-move.test.ts +++ b/stores/__tests__/email-store-cross-account-move.test.ts @@ -80,7 +80,7 @@ describe('email-store moveToMailboxCrossAware', () => { await useEmailStore.getState().moveToMailboxCrossAware(client, 'e1', 'archive-A'); expect(crossSpy).not.toHaveBeenCalled(); - expect(moveSpy).toHaveBeenCalledWith(client, 'e1', 'archive-A'); + expect(moveSpy).toHaveBeenCalledWith(client, 'e1', 'archive-A', undefined); }); it('reverse: shared → own also routes cross-account (source override set)', async () => { diff --git a/stores/email-store.ts b/stores/email-store.ts index 67d765405..181e358bc 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -323,7 +323,8 @@ interface EmailStore { sendRawEmail: (client: IJMAPClient, rawMimeBlob: Blob, identityId: string, delayedUntil?: string, envelopeRecipients?: string[]) => 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; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + markAsRead: (client: IJMAPClient, emailId: string, read: boolean, emailKey?: 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; /** @@ -333,9 +334,10 @@ interface EmailStore { * equivalent of what drag-and-drop already does. Falls back to the plain * single-account `moveToMailbox` when source and destination share an account. */ - moveToMailboxCrossAware: (client: IJMAPClient, emailId: string, mailboxId: string) => Promise; + moveToMailboxCrossAware: (client: IJMAPClient, emailId: string, mailboxId: string, emailKey?: string) => Promise; moveEmailsToMailbox: (client: IJMAPClient, emailIds: string[], mailboxId: string) => Promise; - moveThreadToMailbox: (client: IJMAPClient, emailId: string, mailboxId: string) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + moveThreadToMailbox: (client: IJMAPClient, emailId: string, mailboxId: string, emailKey?: string) => Promise; /** * Take messages out of the open folder only, leaving them in their other * folders (#615). Acts on those of `emails` that removalFolder and @@ -391,15 +393,18 @@ interface EmailStore { scopeSearchToOpenFolder: () => void; clearSearchFilters: () => void; toggleAdvancedSearch: () => void; - toggleStar: (client: IJMAPClient, emailId: string) => Promise; - setEmailKeywords: (client: IJMAPClient, emailId: string, keywords: Record) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + toggleStar: (client: IJMAPClient, emailId: string, emailKey?: string) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + setEmailKeywords: (client: IJMAPClient, emailId: string, keywords: Record, emailKey?: string) => Promise; /** * Set (true) or clear (false) just the named keywords. Every other keyword * is left as the server has it. */ - patchEmailKeywords: (client: IJMAPClient, emailId: string, changes: Record) => Promise; + patchEmailKeywords: (client: IJMAPClient, emailId: string, changes: Record, emailKey?: string) => Promise; markEmailKeyword: (client: IJMAPClient, emailId: string, keyword: string) => Promise; - setEmailKeywordsLocal: (emailId: string, keywords: Record) => void; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + setEmailKeywordsLocal: (emailId: string, keywords: Record, emailKey?: string) => void; // Batch operations /** Acts on the selection, or on `target`'s messages when given. */ @@ -423,8 +428,10 @@ interface EmailStore { // owning JMAP account (passed to JMAP), `sourceClientAccountId` is the login the // email is reachable through (used to pick the right client). (#281) spamUndoCache: Map; - markAsSpam: (client: IJMAPClient, emailId: string) => Promise; - undoSpam: (client: IJMAPClient, emailId: string) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + markAsSpam: (client: IJMAPClient, emailId: string, emailKey?: string) => Promise; + /** `emailKey` (emailKeyFor of the row) picks the right row when the id repeats across accounts. */ + undoSpam: (client: IJMAPClient, emailId: string, emailKey?: string) => Promise; batchMarkAsSpam: (client: IJMAPClient, emailIds: string[]) => Promise; batchUndoSpam: (client: IJMAPClient, emailIds: string[]) => Promise; @@ -532,10 +539,6 @@ function getNextSelectedEmailAfterRemoval(state: { emails: Email[]; selectedEmai return null; } -function getNextSelectedEmail(state: { emails: Email[]; selectedEmail: Email | null }, removedEmailId: string): Email | null { - 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 @@ -1348,10 +1351,14 @@ export function resolveEmailActionContext( function resolveKeywordActionContext( emailId: string, passedClient: IJMAPClient, + emailKey?: string, ): { client: IJMAPClient; accountId: string | undefined } { const state = useEmailStore.getState(); - const email = state.emails.find(e => e.id === emailId) - ?? (state.selectedEmail?.id === emailId ? state.selectedEmail : undefined); + // By the row's key when given: in a merged view the bare id also matches + // another account's namesake, whose account would receive the write. + const isTarget = singleEmailMatcher(emailId, emailKey); + const email = state.emails.find(isTarget) + ?? (state.selectedEmail && isTarget(state.selectedEmail) ? state.selectedEmail : undefined); if (!email) return { client: passedClient, accountId: undefined }; const { client, accountId } = resolveEmailActionContext(email, passedClient); return { client, accountId }; @@ -3378,17 +3385,18 @@ export const useEmailStore = create((set, get) => ({ return true; }, - markAsRead: async (client, emailId, read): Promise => { + markAsRead: async (client, emailId, read, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); try { // Check if this email is already being processed - const processingKey = `${emailId}-${read}`; + const processingKey = `${emailKey ?? emailId}-${read}`; const currentProcessing = get().processingReadStatus; if (currentProcessing.has(processingKey)) { return true; // Already being processed } // Get the email to check its current state and mailboxes - const email = get().emails.find(e => e.id === emailId); + const email = get().emails.find(isTarget); if (!email) return false; // Check if already in the desired state @@ -3425,7 +3433,7 @@ export const useEmailStore = create((set, get) => ({ newProcessingSet.delete(processingKey); // Only update counters if the state is actually changing - const emailInState = state.emails.find(e => e.id === emailId); + const emailInState = state.emails.find(isTarget); if (!emailInState) return { processingReadStatus: newProcessingSet }; const wasRead = emailInState.keywords?.$seen; @@ -3458,9 +3466,9 @@ export const useEmailStore = create((set, get) => ({ return { emails: state.emails.map(e => - e.id === emailId ? { ...e, keywords: { ...e.keywords, $seen: read } } : e + isTarget(e) ? { ...e, keywords: { ...e.keywords, $seen: read } } : e ), - selectedEmail: state.selectedEmail?.id === emailId + selectedEmail: (!!state.selectedEmail && isTarget(state.selectedEmail)) ? { ...state.selectedEmail, keywords: { ...state.selectedEmail.keywords, $seen: read } } : state.selectedEmail, retainedInViewIds, @@ -3476,7 +3484,7 @@ export const useEmailStore = create((set, get) => ({ let updated = false; const newCache = new Map(state.threadEmailsCache); for (const [tid, cachedEmails] of newCache) { - const idx = cachedEmails.findIndex(e => e.id === emailId); + const idx = cachedEmails.findIndex(isTarget); if (idx !== -1) { newCache.set(tid, cachedEmails.map((e, i) => i === idx ? { ...e, keywords: { ...e.keywords, $seen: read } } : e @@ -3506,9 +3514,10 @@ export const useEmailStore = create((set, get) => ({ return true; }, - moveToMailboxCrossAware: async (client, emailId, destinationMailboxId): Promise => { + moveToMailboxCrossAware: async (client, emailId, destinationMailboxId, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); const state = get(); - const email = state.emails.find((e) => e.id === emailId); + const email = state.emails.find(isTarget); if (!email) return false; const { mailboxes } = resolveEmailActionContext(email, client); @@ -3528,7 +3537,7 @@ export const useEmailStore = create((set, get) => ({ sourceMailbox.accountId !== destMailbox.accountId; if (!isCrossAccount) { - return await get().moveToMailbox(client, emailId, destinationMailboxId); + return await get().moveToMailbox(client, emailId, destinationMailboxId, emailKey); } const destAccountId = resolveDestLocalAccountId(destMailbox!); @@ -3536,7 +3545,7 @@ export const useEmailStore = create((set, get) => ({ email.accountId ?? state.viewingAccountId ?? useAuthStore.getState().activeAccountId; if (!destAccountId || !sourceAccountId) { // Can't resolve the local endpoints — fall back rather than drop the mail. - return await get().moveToMailbox(client, emailId, destinationMailboxId); + return await get().moveToMailbox(client, emailId, destinationMailboxId, emailKey); } // JMAP has no cross-account move: copy the raw message into the destination @@ -3971,14 +3980,15 @@ export const useEmailStore = create((set, get) => ({ } }, - moveThreadToMailbox: async (client, emailId, destinationMailboxId): Promise => { + moveThreadToMailbox: async (client, emailId, destinationMailboxId, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); try { const state = get(); - const email = state.emails.find(e => e.id === emailId) - ?? (state.selectedEmail?.id === emailId ? state.selectedEmail : null); + const email = state.emails.find(isTarget) + ?? ((!!state.selectedEmail && isTarget(state.selectedEmail)) ? state.selectedEmail : null); if (!email?.threadId) { - return await get().moveToMailbox(client, emailId, destinationMailboxId); + return await get().moveToMailbox(client, emailId, destinationMailboxId, emailKey); } // In unified view route to the email's own account (client + that @@ -3991,7 +4001,7 @@ export const useEmailStore = create((set, get) => ({ const threadEmailIds = thread?.emailIds?.length ? thread.emailIds : [emailId]; if (threadEmailIds.length <= 1) { - return await get().moveToMailbox(client, emailId, destinationMailboxId); + return await get().moveToMailbox(client, emailId, destinationMailboxId, emailKey); } const move = { emailIds: threadEmailIds, fromMailboxId: moveSourceMailboxId([email], mailboxes), toMailboxId: destinationMailboxId }; @@ -4307,9 +4317,10 @@ export const useEmailStore = create((set, get) => ({ set((state) => ({ isAdvancedSearchOpen: !state.isAdvancedSearchOpen })); }, - toggleStar: async (client, emailId): Promise => { + toggleStar: async (client, emailId, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); try { - const email = get().emails.find(e => e.id === emailId); + const email = get().emails.find(isTarget); if (!email) return false; const isFlagged = email.keywords.$flagged || false; @@ -4325,9 +4336,9 @@ export const useEmailStore = create((set, get) => ({ // Update local state set((state) => ({ emails: state.emails.map(e => - e.id === emailId ? { ...e, keywords: { ...e.keywords, $flagged: !isFlagged } } : e + isTarget(e) ? { ...e, keywords: { ...e.keywords, $flagged: !isFlagged } } : e ), - selectedEmail: state.selectedEmail?.id === emailId + selectedEmail: (!!state.selectedEmail && isTarget(state.selectedEmail)) ? { ...state.selectedEmail, keywords: { ...state.selectedEmail.keywords, $flagged: !isFlagged } } : state.selectedEmail, // In the starred cross view, un-starring drops the mail out of the @@ -4364,7 +4375,8 @@ export const useEmailStore = create((set, get) => ({ void emailHooks.onEmailKeywordChange.emit({ emailIds: [emailId], added: [keyword], removed: [] }); }, - setEmailKeywords: async (client, emailId, keywords): Promise => { + setEmailKeywords: async (client, emailId, keywords, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); // Route the write to the email's OWN account, exactly like toggleStar and // every other single-email action. `resolveEmailActionContext` covers both // shapes: in an aggregate view the owner comes from the email's @@ -4381,54 +4393,56 @@ export const useEmailStore = create((set, get) => ({ // Only the keywords that differ from the row are written (see // patchEmailKeywords): replacing the whole set from the row undid what // another client had set since, such as `$seen` or `$answered`. - const previous = (get().emails.find(e => e.id === emailId) ?? get().selectedEmail)?.keywords ?? {}; + const previous = (get().emails.find(isTarget) ?? get().selectedEmail)?.keywords ?? {}; const changes: Record = {}; for (const key of new Set([...Object.keys(previous), ...Object.keys(keywords)])) { if (!!previous[key] !== !!keywords[key]) changes[key] = !!keywords[key]; } - return await get().patchEmailKeywords(client, emailId, changes); + return await get().patchEmailKeywords(client, emailId, changes, emailKey); }, - patchEmailKeywords: async (client, emailId, changes): Promise => { + patchEmailKeywords: async (client, emailId, changes, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); const keys = Object.keys(changes); if (keys.length === 0) return true; if (emailHooks.onBeforeEmailKeywordChange.size > 0 && !(await emailHooks.onBeforeEmailKeywordChange.intercept({ emailIds: [emailId], ...keywordChangeLists(changes) }))) { return false; } - const { client: actionClient, accountId } = resolveKeywordActionContext(emailId, client); + const { client: actionClient, accountId } = resolveKeywordActionContext(emailId, client, emailKey); // `keywords/` pointers touch only these keywords on the server; // a cleared keyword is removed (null), since a keyword map holds `true` // values only. const patch = Object.fromEntries(keys.map(key => [keywordPointer(key), changes[key] ? true : null])); await actionClient.batchUpdateKeywords([emailId], patch, accountId); - const previous = (get().emails.find(e => e.id === emailId) ?? get().selectedEmail)?.keywords ?? {}; + const previous = (get().emails.find(isTarget) ?? get().selectedEmail)?.keywords ?? {}; const next: Record = { ...previous }; for (const key of keys) { if (changes[key]) next[key] = true; else delete next[key]; } - get().setEmailKeywordsLocal(emailId, next); + get().setEmailKeywordsLocal(emailId, next, emailKey); void emailHooks.onEmailKeywordChange.emit({ emailIds: [emailId], ...keywordChangeLists(changes) }); refillAfterKeywordChange(get, client, keys.filter(k => !!previous[k] !== !!changes[k])); return true; }, - setEmailKeywordsLocal: (emailId, keywords) => { + setEmailKeywordsLocal: (emailId, keywords, emailKey) => { + const isTarget = singleEmailMatcher(emailId, emailKey); set((state) => { // This patch replaces the whole keyword map, so it can flip $seen as well // as labels. Only a genuine read-state change moves the tag unread counts. - const previous = state.emails.find(e => e.id === emailId) ?? state.selectedEmail; + const previous = state.emails.find(isTarget) ?? state.selectedEmail; const wasRead = previous?.keywords?.$seen ?? false; const isRead = keywords.$seen ?? false; const delta = wasRead === isRead ? 0 : (isRead ? -1 : 1); return { emails: state.emails.map(e => - e.id === emailId ? { ...e, keywords: { ...keywords } } : e + isTarget(e) ? { ...e, keywords: { ...keywords } } : e ), - selectedEmail: state.selectedEmail?.id === emailId + selectedEmail: (!!state.selectedEmail && isTarget(state.selectedEmail)) ? { ...state.selectedEmail, keywords: { ...keywords } } : state.selectedEmail, tagCounts: applyTagCountReadDelta(state.tagCounts, [{ keywords, delta }]), @@ -5127,14 +5141,15 @@ export const useEmailStore = create((set, get) => ({ }, // Spam operations - markAsSpam: async (client, emailId): Promise => { + markAsSpam: async (client, emailId, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); // The viewer may show an email the list no longer holds (e.g. the unread // quick-filter dropped it once it was read), so fall back to the selected // email like moveThreadToMailbox does. Throw rather than no-op so the // caller's success toast cannot fire for a message that was not moved. (#695) const state = get(); - const email = state.emails.find(e => e.id === emailId) - ?? (state.selectedEmail?.id === emailId ? state.selectedEmail : null); + const email = state.emails.find(isTarget) + ?? ((!!state.selectedEmail && isTarget(state.selectedEmail)) ? state.selectedEmail : null); if (!email) { throw new Error('Email not found - cannot mark as spam'); } @@ -5159,7 +5174,7 @@ export const useEmailStore = create((set, get) => ({ // still move the message, and undo falls back to the inbox. Returning // here let the caller show a success toast for a message never moved. if (currentMailbox) { - get().spamUndoCache.set(emailId, { + get().spamUndoCache.set(emailKey ?? emailId, { emailId, originalMailboxId: currentMailbox.originalId || currentMailbox.id, accountId, @@ -5205,8 +5220,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, }; }); @@ -5218,7 +5233,8 @@ export const useEmailStore = create((set, get) => ({ return true; }, - undoSpam: async (client, emailId): Promise => { + undoSpam: async (client, emailId, emailKey): Promise => { + const isTarget = singleEmailMatcher(emailId, emailKey); if (emailHooks.onBeforeEmailSpamToggle.size > 0 && !(await emailHooks.onBeforeEmailSpamToggle.intercept({ emailIds: [emailId], spam: false }))) { return false; @@ -5227,7 +5243,7 @@ export const useEmailStore = create((set, get) => ({ const mailboxes = resolveActionMailboxes(); // Try cache first (preserves exact original mailbox for toast undo) - const cachedData = get().spamUndoCache.get(emailId); + const cachedData = get().spamUndoCache.get(emailKey ?? emailId); let targetMailboxId: string; let accountId: string | undefined; @@ -5242,13 +5258,13 @@ export const useEmailStore = create((set, get) => ({ if (cachedData.sourceClientAccountId) { undoClient = useAuthStore.getState().getClientForAccount(cachedData.sourceClientAccountId) ?? undoClient; } - get().spamUndoCache.delete(emailId); + get().spamUndoCache.delete(emailKey ?? emailId); } else { // Generic "not spam" (button/menu, no undo cache). In aggregate views // (e.g. "All Junk") route to the email's own account so shared/group // messages move back to *their* inbox via *their* client, not the active // account's. (#281) - const listEmail = get().emails.find(e => e.id === emailId); + const listEmail = get().emails.find(isTarget); let inboxMailboxes = mailboxes; if (listEmail?.sourceClientAccountId && listEmail?.sourceAccountId) { undoClient = useAuthStore.getState().getClientForAccount(listEmail.sourceClientAccountId) ?? undoClient; @@ -5279,8 +5295,8 @@ export const useEmailStore = create((set, get) => ({ // message to the next one, mirroring markAsSpam. Without this the viewer // stays stuck on the mail that just left the folder. set(state => ({ - emails: state.emails.filter(e => e.id !== emailId), - selectedEmail: getNextSelectedEmail(state, emailId), + emails: state.emails.filter(e => !isTarget(e)), + selectedEmail: getNextSelectedEmailAfterRemovingRow(state, isTarget), })); void emailHooks.onEmailSpamToggle.emit({ emailIds: [emailId], spam: false });