From 5e42c6737e697d69efda22eae5e0958222e29dc9 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 18:53:23 +0200 Subject: [PATCH] fix: report spam from the viewer the way the list does #695 The viewer's Spam and Not spam buttons were a plain move into Junk or Inbox: they sent no `$junk`/`$notjunk` flip, so the server's classifier never learned from them, and they ignored the "trash-and-read" delete action. They now call the store's `markSpam`/`unmarkSpam`, the path the list and swipe actions use, which take the viewed message and file it in its own account's Junk or Inbox, also when the list does not hold it. Not spam is only offered when that account has an Inbox. --- src/screens/EmailThreadScreen.tsx | 23 +++++----- .../email-store-viewer-actions.test.ts | 41 ++++++++++++++++- src/stores/email-store.ts | 44 ++++++++++++++----- 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/src/screens/EmailThreadScreen.tsx b/src/screens/EmailThreadScreen.tsx index ccf641de..6945405e 100644 --- a/src/screens/EmailThreadScreen.tsx +++ b/src/screens/EmailThreadScreen.tsx @@ -61,6 +61,8 @@ export default function EmailThreadScreen({ route, navigation }: Props) { const deleteEmail = useEmailStore((s) => s.deleteEmail); const moveToMailbox = useEmailStore((s) => s.moveToMailbox); const archiveEmailAction = useEmailStore((s) => s.archiveEmail); + const markSpam = useEmailStore((s) => s.markSpam); + const unmarkSpam = useEmailStore((s) => s.unmarkSpam); const mailboxes = useEmailStore((s) => s.mailboxes); const currentMailboxId = useEmailStore((s) => s.currentMailboxId); const disableThreading = useSettingsStore((s) => s.disableThreading); @@ -318,6 +320,8 @@ export default function EmailThreadScreen({ route, navigation }: Props) { // offered for mail of that account. const canArchive = !!archiveMailbox && listAccountId === ownerAccountId && sourceMailbox?.id !== archiveMailbox.id; + // Not spam files back into the Inbox, so it needs one. + const canToggleSpam = isInJunk ? !!inboxMailbox : !!junkMailbox; const onToggleKeyword = (token: string) => { if (!email) return; @@ -397,18 +401,15 @@ export default function EmailThreadScreen({ route, navigation }: Props) { navigation.goBack(); }; + // The same store path as the list and swipe actions: files into the + // message's own Junk or Inbox, flips `$junk`/`$notjunk` and honours + // trash-and-read (#695). const onToggleSpam = () => { - if (!email || !sourceMailbox) return; + if (!email || !canToggleSpam) return; setMoreMenuOpen(false); const viewed = { email, accountId: ownerAccountId }; - if (isInJunk) { - const target = inboxMailbox ?? scopedMailboxes.find((m) => m.id !== sourceMailbox.id); - if (!target) return; - void moveToMailbox(email.id, sourceMailbox.id, target.id, viewed); - } else { - if (!junkMailbox) return; - void moveToMailbox(email.id, sourceMailbox.id, junkMailbox.id, viewed); - } + if (isInJunk) void unmarkSpam([email.id], viewed); + else void markSpam([email.id], viewed); navigation.goBack(); }; @@ -544,7 +545,7 @@ export default function EmailThreadScreen({ route, navigation }: Props) { icon: (s, col) => isInJunk ? : , onPress: onToggleSpam, - available: !!junkMailbox && !!sourceMailbox, + available: canToggleSpam, }, tag: { label: t('email_viewer.tag', 'Tag'), @@ -721,7 +722,7 @@ export default function EmailThreadScreen({ route, navigation }: Props) { canArchive={canArchive} canMarkUnread={true} canMove={!!sourceMailbox} - showSpam={!!junkMailbox && !!sourceMailbox} + showSpam={canToggleSpam} isInJunk={isInJunk} canViewSource={!!email?.blobId} canExport={!!email?.blobId} diff --git a/src/stores/__tests__/email-store-viewer-actions.test.ts b/src/stores/__tests__/email-store-viewer-actions.test.ts index 17432f64..e4ce52fb 100644 --- a/src/stores/__tests__/email-store-viewer-actions.test.ts +++ b/src/stores/__tests__/email-store-viewer-actions.test.ts @@ -95,11 +95,14 @@ vi.mock('../../api/jmap-client', () => ({ import * as emailApi from '../../api/email'; import { useEmailStore, type ViewedEmail } from '../email-store'; +import { useSettingsStore } from '../settings-store'; import type { Email, Mailbox } from '../../api/types'; const mockDeleteEmail = emailApi.deleteEmail as ReturnType; const mockMoveEmail = emailApi.moveEmail as ReturnType; const mockRestore = emailApi.restoreEmailMailboxes as ReturnType; +const mockMarkAsSpam = emailApi.markAsSpam as ReturnType; +const mockUndoSpam = emailApi.undoSpam as ReturnType; function own(id: string, role: string): Mailbox { return { id, name: role, role, accountId: 'acc-1', isShared: false } as Mailbox; @@ -109,8 +112,8 @@ function team(rawId: string, role: string): Mailbox { } const MAILBOXES = [ - own('a', 'inbox'), own('t', 'trash'), own('x', 'archive'), - team('a', 'inbox'), team('t', 'trash'), team('x', 'archive'), + own('a', 'inbox'), own('t', 'trash'), own('x', 'archive'), own('j', 'junk'), + team('a', 'inbox'), team('t', 'trash'), team('x', 'archive'), team('j', 'junk'), ]; // The user's own "Security audit #18" and the team's message share the id. const OWN_ROW = { id: 'e1', subject: 'Security audit #18', keywords: {}, mailboxIds: { a: true } } as unknown as Email; @@ -188,3 +191,37 @@ describe('viewer actions on another account\'s message (B3)', () => { expect(useEmailStore.getState().emails).toEqual([]); }); }); + +describe('viewer Spam takes the list and swipe path (#695)', () => { + it('files a team message into the team Junk flipping $junk/$notjunk, and leaves the list alone', async () => { + useEmailStore.setState({ mailboxes: MAILBOXES, currentMailboxId: 'a', emails: [OWN_ROW] }); + + await useEmailStore.getState().markSpam(['e1'], TEAM_VIEWED); + + expect(mockMarkAsSpam).toHaveBeenCalledWith(['e1'], 'j', 'grp-1', { markRead: false }); + expect(useEmailStore.getState().emails).toEqual([OWN_ROW]); + const undo = useEmailStore.getState().pendingUndo!; + expect(undo).toMatchObject({ kind: 'spam', accountId: 'grp-1', keywordPatch: { $junk: true, $notjunk: null } }); + expect(undo.items[0].originalMailboxIds).toEqual({ a: true }); + }); + + it('honours trash-and-read for a message the list does not hold', async () => { + useSettingsStore.getState().updateSetting('deleteAction', 'trash-and-read'); + useEmailStore.setState({ mailboxes: MAILBOXES, currentMailboxId: 'a', emails: [] }); + + await useEmailStore.getState().markSpam(['e1'], { email: OWN_ROW, accountId: undefined }); + + expect(mockMarkAsSpam).toHaveBeenCalledWith(['e1'], 'j', undefined, { markRead: true }); + useSettingsStore.getState().updateSetting('deleteAction', 'trash'); + }); + + it('files a team message back into the team Inbox with $notjunk', async () => { + useEmailStore.setState({ mailboxes: MAILBOXES, currentMailboxId: 'a', emails: [OWN_ROW] }); + const junked = { ...TEAM_MESSAGE, keywords: { $junk: true }, mailboxIds: { j: true } } as unknown as Email; + + await useEmailStore.getState().unmarkSpam(['e1'], { email: junked, accountId: 'grp-1' }); + + expect(mockUndoSpam).toHaveBeenCalledWith(['e1'], 'a', 'grp-1'); + expect(useEmailStore.getState().emails).toEqual([OWN_ROW]); + }); +}); diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index 0d73b8c4..7247487c 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -34,7 +34,7 @@ import { } from '../api/email'; import { useNetworkStore } from './network-store'; import { JMAPMethodError } from '../api/jmap-result'; -import { mailboxesForSiblingOf, findJunkMailbox } from '../lib/mailbox-tree'; +import { mailboxesForSiblingOf, mailboxesOfAccount, findJunkMailbox } from '../lib/mailbox-tree'; import { toWildcardQuery } from '../lib/search-utils'; import { orderForMailbox, sanitizeSortLevels, type SortLevel } from '../lib/message-list-order'; import { buildListSort, markKeywordSortUnsupported } from '../lib/keyword-sort-polarity'; @@ -156,6 +156,25 @@ function actionTarget( return { email: row ?? viewed?.email, listed }; } +// actionTarget for the selection actions: the viewer only acts on its one message. +function actionTargets( + state: EmailState, + emailIds: string[], + viewed?: ViewedEmail, +): { targets: Email[]; listed: boolean } { + if (!viewed) return { targets: state.emails.filter((e) => emailIds.includes(e.id)), listed: true }; + const { email, listed } = actionTarget(state, viewed.email.id, viewed); + return { targets: email ? [email] : [], listed }; +} + +// Where an action looks up Junk, Inbox or Archive: the viewed message's +// account, otherwise the open folder's. +function actionMailboxes(state: EmailState, viewed?: ViewedEmail): Mailbox[] { + return viewed + ? mailboxesOfAccount(state.mailboxes, viewed.accountId) + : mailboxesForSiblingOf(state.mailboxes, state.currentMailboxId); +} + // Refuse a viewer action aimed at folders outside the message's account: // the same id there is another message. function assertViewedAccount(viewed: ViewedEmail | undefined, ...refs: MailboxRef[]): void { @@ -354,11 +373,12 @@ export interface EmailState { /** * File messages into the current account's Junk and flip `$junk`/`$notjunk` * (#850); honours the "trash-and-read" delete action by also marking read. - * Works for one id or a selection; offers undo. + * Works for one id or a selection; offers undo. With `viewed`, the viewer's + * message goes to its own account's Junk (#695). */ - markSpam: (emailIds: string[]) => Promise; + markSpam: (emailIds: string[], viewed?: ViewedEmail) => Promise; /** Inverse of markSpam: back to Inbox with `$notjunk`. */ - unmarkSpam: (emailIds: string[]) => Promise; + unmarkSpam: (emailIds: string[], viewed?: ViewedEmail) => Promise; // ── Batch (multi-select) actions ────────────────────────────── archiveEmailsBatch: (emailIds: string[]) => Promise; moveEmailsToMailbox: (emailIds: string[], toMailboxId: string) => Promise; @@ -1088,11 +1108,11 @@ export const useEmailStore = create()( patchCache(emailId, { keywords: patch }, currentAccountId(state)); }, - markSpam: async (emailIds) => { + markSpam: async (emailIds, viewed) => { const state = get(); - const targets = state.emails.filter((e) => emailIds.includes(e.id)); + const { targets, listed } = actionTargets(state, emailIds, viewed); if (targets.length === 0) return; - const scoped = mailboxesForSiblingOf(state.mailboxes, state.currentMailboxId); + const scoped = actionMailboxes(state, viewed); const junkMailbox = findJunkMailbox(scoped); if (!junkMailbox) { set({ error: t('email_list.no_junk_folder', 'Could not find a Spam/Junk folder on the server.') }); @@ -1116,7 +1136,7 @@ export const useEmailStore = create()( () => apiMarkAsSpam(targets.map((e) => e.id), junk.id, junk.accountId, { markRead }), ); - const removed = new Set(targets.map((e) => e.id)); + const removed = new Set(listed ? targets.map((e) => e.id) : []); set({ emails: get().emails.filter((e) => !removed.has(e.id)), pendingUndo: { @@ -1133,11 +1153,11 @@ export const useEmailStore = create()( for (const e of targets) patchCache(e.id, { mailboxIds: junkTarget, keywords: keywordPatch }, junk.accountId); }, - unmarkSpam: async (emailIds) => { + unmarkSpam: async (emailIds, viewed) => { const state = get(); - const targets = state.emails.filter((e) => emailIds.includes(e.id)); + const { targets, listed } = actionTargets(state, emailIds, viewed); if (targets.length === 0) return; - const scoped = mailboxesForSiblingOf(state.mailboxes, state.currentMailboxId); + const scoped = actionMailboxes(state, viewed); const inboxMailbox = scoped.find((m) => m.role === 'inbox'); if (!inboxMailbox) return; const inbox = refFor(state.mailboxes, inboxMailbox.id); @@ -1157,7 +1177,7 @@ export const useEmailStore = create()( () => apiUndoSpam(targets.map((e) => e.id), inbox.id, inbox.accountId), ); - const removed = new Set(targets.map((e) => e.id)); + const removed = new Set(listed ? targets.map((e) => e.id) : []); set({ emails: get().emails.filter((e) => !removed.has(e.id)), pendingUndo: {