From 37dc08160020bb92bdd23c2286d2f21a7b33b358 Mon Sep 17 00:00:00 2001 From: Andrei Hajdukewycz Date: Tue, 16 Jun 2026 05:05:36 +0000 Subject: [PATCH 1/2] Add bulk "Not junk" whitelist for multi-selected Junk messages. Multi-selecting in the Junk folder can now whitelist every selected message's sender and rescue the batch to the Inbox. The trust write is batched into a single ContactCard/set (one book lookup, one existence query, one multi-create) rather than a per-sender loop, and the single-message whitelist becomes the N=1 case of the batched path. Adds unit coverage for the batched trust helper and the store dedup/notice, and an e2e for the multi-select flow. --- src/components/MessageView.vue | 30 +++++ src/stores/mail-store.ts | 173 ++++++++++++++++++-------- src/sync/backends/jmap/contacts.ts | 144 ++++++++++++++++++--- src/sync/backends/jmap/outbox.ts | 37 +++--- tests/e2e/contacts-junk.spec.js | 128 ++++++++++++++++++- tests/unit/stores/mail-store.test.ts | 64 ++++++++++ tests/unit/sync/jmap-contacts.test.ts | 96 ++++++++++++++ 7 files changed, 579 insertions(+), 93 deletions(-) diff --git a/src/components/MessageView.vue b/src/components/MessageView.vue index d9132d5..be5e431 100644 --- a/src/components/MessageView.vue +++ b/src/components/MessageView.vue @@ -537,6 +537,23 @@ async function bulkDelete() { } } +// Bulk "Not junk": whitelist every selected message's sender and move +// the batch to the Inbox. Junk-folder only, gated in the template. +const bulkWhitelisting = ref(false); + +async function bulkWhitelist() { + const ids = [...mailStore.selectedIds]; + if (ids.length === 0 || bulkWhitelisting.value) return; + bulkWhitelisting.value = true; + try { + await mailStore.whitelistSenders(ids); + } catch (err) { + console.warn('[message-view] bulk whitelist failed', err?.message ?? err); + } finally { + bulkWhitelisting.value = false; + } +} + function clearBulkSelection() { mailStore.clearSelection(); } @@ -563,6 +580,19 @@ function closeMessageView() { {{ selectionCount }} {{ selectionCount === 1 ? 'message' : 'messages' }} selected
+ + diff --git a/src/stores/mail-store.ts b/src/stores/mail-store.ts index 806184e..16dae08 100644 --- a/src/stores/mail-store.ts +++ b/src/stores/mail-store.ts @@ -1356,84 +1356,146 @@ export const useMailStore = defineStore('mail', () => { } /** - * Whitelist the sender of a Junk message (Strategy C from - * whitelist-in-webmail-notes.md): + * Whitelist the senders of one or more Junk messages and rescue the + * messages (Strategy C from whitelist-in-webmail-notes.md): * - * 1. Trust the sender for future mail — queue a whitelistSender - * mutation that adds a ContactCard in the "Trusted senders" - * address book; Stalwart's trustContacts / card_is_ham then - * delivers future authenticated mail from that address to the - * Inbox. - * 2. Rescue the current message — remove $junk / add $notjunk - * (optimistic + queued setKeywords) and move Junk → Inbox, since - * contact trust only applies at ingest time for future mail. + * 1. Trust the senders for future mail — queue one whitelistSender + * mutation carrying the unique sender addresses; the outbox adds a + * ContactCard per address in the "Trusted senders" address book so + * Stalwart's trustContacts / card_is_ham delivers future + * authenticated mail from them to the Inbox, and reconciles the + * contacts cache once. + * 2. Rescue the messages — remove $junk / add $notjunk (optimistic + + * one queued setKeywords for the batch) and move Junk → Inbox, + * since contact trust only applies at ingest time for future mail. * - * Only meaningful from the Junk folder; the UI gates the button on - * the junk role. Surfaces a transient success notice when the sender - * was trusted and the message moved, or an error when the message - * moved but the trust write did not apply. + * A message whose From header yields no address is still rescued; its + * sender is just skipped for trust. Only meaningful from the Junk + * folder; the UI gates the action on the junk role. Surfaces a + * transient success notice when the senders were trusted and the + * messages moved, or an error when the messages moved but the trust + * write did not apply. */ - async function whitelistSender(messageId: number) { + async function whitelistSenders(ids: number[]): Promise { if (!repo || authStore.accountId == null) return { succeeded: 0, failed: 0, skipped: 0 }; - const row = messages.value.find((m) => m?.id === messageId); - if (!row) return { succeeded: 0, failed: 0, skipped: 0 }; - const sender = parseSender(row.from_text); - if (!sender) { - error.value = 'Could not determine the sender to whitelist.'; - return { succeeded: 0, failed: 0, skipped: 0 }; - } + const messageIds = normalizeMessageIds(ids); + if (messageIds.length === 0) return { succeeded: 0, failed: 0, skipped: 0 }; const target = inbox.value; if (!target?.id) { error.value = 'No Inbox folder is configured.'; return { succeeded: 0, failed: 0, skipped: 0 }; } - // 1) Trust the sender going forward. Run the mutation (rather than - // fire-and-forget) so the confirmation can reflect whether the - // trust write actually applied — the whole point of the action is - // that future mail is trusted, so we must not claim success when - // only the message move happened. - const trustMutation = await repo.insertPendingMutation({ - accountId: authStore.accountId, - mutationType: MUTATION_TYPE.WHITELIST_SENDER, - targetMessageId: messageId, - requestJson: JSON.stringify({ email: sender.email, name: sender.name }), + // Gather the live rows and the unique senders to trust (deduped by + // address, case-insensitively). Rows with an unparseable From are + // still rescued below; they just contribute no trusted sender. + const rows: CachedRow[] = []; + const sendersByEmail = new Map(); + for (const id of messageIds) { + const row = messages.value.find((m) => m?.id === id); + if (!row) continue; + rows.push(row); + const sender = parseSender(row.from_text); + if (sender && !sendersByEmail.has(sender.email.toLowerCase())) { + sendersByEmail.set(sender.email.toLowerCase(), sender); + } + } + if (rows.length === 0) return { succeeded: 0, failed: 0, skipped: messageIds.length }; + const rescueIds = rows.map((r) => r.id); + const senders = [...sendersByEmail.values()]; + + // 1) Trust every unique sender in a single mutation, run it, and + // capture whether the trust write applied — the whole point of the + // action is that future mail is trusted, so we must not claim + // success when only the message move happened. + let trusted = true; + if (senders.length > 0) { + const trustMutation = await repo.insertPendingMutation({ + accountId: authStore.accountId, + mutationType: MUTATION_TYPE.WHITELIST_SENDER, + targetMessageId: null, + requestJson: JSON.stringify({ senders }), + }); + const trustResult = typeof repo.runMutation === 'function' && trustMutation?.id != null + ? await repo.runMutation(authStore.accountId, trustMutation.id) + : await repo.drainOutbox(authStore.accountId); + // A run that attempted nothing is not a success; mirror runChunkedMutation. + trusted = (trustResult?.failed ?? 0) === 0 + && ((trustResult?.attempted ?? 0) > 0 || (trustResult?.succeeded ?? 0) > 0); + } + + // 2a) Rescue the selected messages' spam keywords: one optimistic + // transaction plus one queued setKeywords for the whole batch. + const optimisticItems = rows.map((row) => { + const keywordsJson = JSON.parse(row.keywords_json ?? '{}'); + delete keywordsJson.$junk; + keywordsJson.$notjunk = true; + return { + messageId: row.id, + keywords: Object.keys(keywordsJson), + keywordsJson: JSON.stringify(keywordsJson), + }; }); - const trustResult = typeof repo.runMutation === 'function' && trustMutation?.id != null - ? await repo.runMutation(authStore.accountId, trustMutation.id) - : await repo.drainOutbox(authStore.accountId); - // A run that attempted nothing (e.g. the row never reached a - // runnable state) is not a success; mirror runChunkedMutation. - const trusted = (trustResult?.failed ?? 0) === 0 - && ((trustResult?.attempted ?? 0) > 0 || (trustResult?.succeeded ?? 0) > 0); - - // 2a) Rescue the current message's spam keywords (optimistic + queued). - const keywordsJson = JSON.parse(row.keywords_json ?? '{}'); - delete keywordsJson.$junk; - keywordsJson.$notjunk = true; - await repo.replaceMessageKeywords(messageId, Object.keys(keywordsJson), JSON.stringify(keywordsJson)); + if (typeof repo.replaceMessageKeywordsMany === 'function') { + await repo.replaceMessageKeywordsMany(optimisticItems); + } else { + for (const item of optimisticItems) { + await repo.replaceMessageKeywords(item.messageId, item.keywords, item.keywordsJson); + } + } await repo.insertPendingMutation({ accountId: authStore.accountId, mutationType: MUTATION_TYPE.SET_KEYWORDS, - targetMessageId: messageId, - requestJson: JSON.stringify({ add: ['$notjunk'], remove: ['$junk'] }), + targetMessageId: rescueIds.length === 1 ? rescueIds[0] : null, + requestJson: JSON.stringify({ messageIds: rescueIds, add: ['$notjunk'], remove: ['$junk'] }), }); - // 2b) Move it out of Junk into the Inbox (the visible effect). - const result = await moveMessages([messageId], target.id); + // 2b) Move them all out of Junk into the Inbox (the visible effect). + const result = await moveMessages(rescueIds, target.id); if (result.succeeded > 0) { - const who = sender.name ?? sender.email; - if (trusted) { - setNotice(`Whitelisted ${who} — moved to Inbox`); + const movedPhrase = result.succeeded === 1 + ? 'moved to Inbox' + : `moved ${result.succeeded} messages to Inbox`; + if (senders.length === 0) { + setNotice(`Moved ${result.succeeded} ${result.succeeded === 1 ? 'message' : 'messages'} to Inbox`); + } else if (trusted) { + const who = senders.length === 1 + ? (senders[0].name ?? senders[0].email) + : `${senders.length} senders`; + setNotice(`Whitelisted ${who} — ${movedPhrase}`); } else { - // The message still moved, but the trust write did not apply — - // don't tell the user the sender was whitelisted. - error.value = `Moved to Inbox, but ${who} could not be added to your trusted contacts — future mail from them may still be treated as junk.`; + // Messages moved, but the trust write did not apply — don't claim + // the senders were whitelisted. + const what = result.succeeded === 1 + ? 'Moved to Inbox' + : `Moved ${result.succeeded} messages to Inbox`; + const who = senders.length === 1 + ? (senders[0].name ?? senders[0].email) + : 'the senders'; + error.value = `${what}, but ${who} could not be added to your trusted contacts — future mail from them may still be treated as junk.`; } } return result; } + /** + * Whitelist the sender of a single Junk message and rescue it to the + * Inbox. Strict about the sender: when the From header yields no + * address there is nothing to whitelist, so it reports that rather than + * silently moving the message. Shares the batch work with + * whitelistSenders. + */ + async function whitelistSender(messageId: number): Promise { + if (!repo || authStore.accountId == null) return { succeeded: 0, failed: 0, skipped: 0 }; + const row = messages.value.find((m) => m?.id === messageId); + if (!row) return { succeeded: 0, failed: 0, skipped: 0 }; + if (!parseSender(row.from_text)) { + error.value = 'Could not determine the sender to whitelist.'; + return { succeeded: 0, failed: 0, skipped: 0 }; + } + return whitelistSenders([messageId]); + } + /** * Delete one or more messages. Single-row delete from the open * message and multi-select bulk delete go through the same path, @@ -2149,6 +2211,7 @@ export const useMailStore = defineStore('mail', () => { moveMessages, archiveMessages, whitelistSender, + whitelistSenders, canMoveToFolder, clearSelection, refresh, diff --git a/src/sync/backends/jmap/contacts.ts b/src/sync/backends/jmap/contacts.ts index 519989f..a31017b 100644 --- a/src/sync/backends/jmap/contacts.ts +++ b/src/sync/backends/jmap/contacts.ts @@ -245,6 +245,8 @@ interface ContactWriteResult { ok: boolean; error?: ContactWriteError; id?: string; + ids?: string[]; + created?: number; alreadyExists?: boolean; alreadyTrusted?: boolean; } @@ -396,22 +398,77 @@ export async function ensureTrustedSendersBook({ transport, account, useWebSocke /** * Add a sender to the trusted-senders address book as a ContactCard so * Stalwart delivers future authenticated mail from that address to the - * Inbox (trustContacts). Idempotent: skips the create when a card - * already exists for the address. Returns { ok, alreadyTrusted?, id?, - * error? }. + * Inbox (trustContacts). Single-sender convenience wrapper over + * createTrustedContactCards; idempotent (skips an address that already + * has a card). */ export async function createTrustedContactCard({ transport, account, email, name, useWebSocket = false, }): Promise { - const address = String(email ?? '').trim(); - if (!address) { + return createTrustedContactCards({ + transport, account, senders: [{ email, name }], useWebSocket, + }); +} + +/** + * Trust one or more senders in a constant number of round trips rather + * than one ContactCard/set per sender: a single existence query + * (ContactCard/query OR over the addresses + one ContactCard/get), a + * single trusted-senders book lookup, and a single multi-create + * ContactCard/set. Idempotent — addresses that already have a card + * anywhere in the account are skipped, so a retry after a partial failure + * converges. Returns { ok, created, alreadyTrusted?, ids?, error? }. + */ +export async function createTrustedContactCards({ + transport, account, senders, useWebSocket = false, +}): Promise { + // Dedupe by address (case-insensitive), keeping the first name seen. + const byEmail = new Map(); + for (const s of senders ?? []) { + const email = String(s?.email ?? '').trim(); + if (!email) continue; + const key = email.toLowerCase(); + if (!byEmail.has(key)) byEmail.set(key, { email, name: s?.name ?? null }); + } + const unique = [...byEmail.values()]; + if (unique.length === 0) { return { ok: false, error: { type: 'invalidArguments', message: 'no sender email' } }; } - if (await cardExistsForEmail({ transport, account, email: address, useWebSocket })) { - return { ok: true, alreadyTrusted: true }; + + // 1) One existence query for every address; skip ones already carded. + const existing = await existingCardEmails({ + transport, account, emails: unique.map((s) => s.email), useWebSocket, + }); + const toCreate = unique.filter((s) => !existing.has(s.email.toLowerCase())); + if (toCreate.length === 0) { + return { ok: true, created: 0, alreadyTrusted: true }; } + + // 2) Resolve the trusted-senders book once for the whole batch. const bookId = await ensureTrustedSendersBook({ transport, account, useWebSocket }); - return submitContactCardCreate({ transport, account, emails: [address], name, bookId, useWebSocket }); + + // 3) Create every missing card in a single ContactCard/set. + const create: Record = {}; + toCreate.forEach((s, i) => { + create[`c${i + 1}`] = buildContactCard({ name: s.name, emails: [s.email], bookId }); + }); + const result = await callJmap(transport, { + using: [JMAP_CAPS.CORE, JMAP_CAPS.CONTACTS], + methodCalls: [[ + 'ContactCard/set', + { accountId: account.remote_account_id, create }, + 'cset', + ]], + useWebSocket, + }); + const set = pickResponse(result, 'ContactCard/set'); + if (!set) return { ok: false, error: { type: 'serverFail' } }; + const notCreated = set.notCreated ? Object.values(set.notCreated) : []; + if (notCreated.length > 0) { + return { ok: false, error: { type: 'notCreated', detail: notCreated[0] } }; + } + const ids = Object.values(set.created ?? {}).map((c: any) => c?.id).filter(Boolean); + return { ok: true, created: ids.length, ids }; } /** @@ -671,18 +728,64 @@ async function cardExistsForEmail({ transport, account, email, useWebSocket }): } /** - * Low-level ContactCard/set create shared by the whitelist and contacts - * UI paths. Builds the JSContact map shape Stalwart accepts. `emails` is - * an ordered, already-normalized list of addresses (at least one). + * Of the given addresses, return the set (lowercased) that already have a + * ContactCard anywhere in the account — in two calls regardless of count: + * one ContactCard/query (OR over the addresses) and one ContactCard/get. + * A filter the server does not support yields no ids, so callers fall + * through to create rather than failing. */ -async function submitContactCardCreate({ - transport, account, emails, name, bookId, useWebSocket, -}): Promise { +async function existingCardEmails({ transport, account, emails, useWebSocket }): Promise> { + const present = new Set(); + if (!emails || emails.length === 0) return present; + const wanted = new Set(emails.map((e) => e.toLowerCase())); + const filter = emails.length === 1 + ? { email: emails[0] } + : { operator: 'OR', conditions: emails.map((email) => ({ email })) }; + const found = await callJmap(transport, { + using: [JMAP_CAPS.CORE, JMAP_CAPS.CONTACTS], + methodCalls: [[ + 'ContactCard/query', + { accountId: account.remote_account_id, filter }, + 'cq', + ]], + useWebSocket, + }); + const ids = pickResponse(found, 'ContactCard/query')?.ids ?? []; + if (ids.length === 0) return present; + const got = await callJmap(transport, { + using: [JMAP_CAPS.CORE, JMAP_CAPS.CONTACTS], + methodCalls: [[ + 'ContactCard/get', + { accountId: account.remote_account_id, ids, properties: ['emails'] }, + 'cg', + ]], + useWebSocket, + }); + const cards = pickResponse(got, 'ContactCard/get')?.list ?? []; + for (const card of cards) { + const map = card?.emails; + if (!map || typeof map !== 'object') continue; + for (const entry of Object.values(map) as any[]) { + const addr = String(entry?.address ?? '').trim().toLowerCase(); + if (addr && wanted.has(addr)) present.add(addr); + } + } + return present; +} + +/** + * Build the JSContact `Card` shape Stalwart accepts for a create, shared + * by the single-add and batched create paths. `emails` is an ordered, + * already-normalized list of addresses (at least one). + */ +function buildContactCard({ + name, emails, bookId, +}: { name?: string | null; emails: string[]; bookId?: string | null }): Record { const emailsMap: Record = {}; emails.forEach((address, i) => { emailsMap[`e${i + 1}`] = { '@type': 'EmailAddress', address }; }); - const card = { + return { '@type': 'Card', version: '1.0', kind: 'individual', @@ -690,6 +793,17 @@ async function submitContactCardCreate({ emails: emailsMap, ...(bookId ? { addressBookIds: { [bookId]: true } } : {}), }; +} + +/** + * Low-level ContactCard/set create shared by the whitelist and contacts + * UI paths. Builds the JSContact map shape Stalwart accepts. `emails` is + * an ordered, already-normalized list of addresses (at least one). + */ +async function submitContactCardCreate({ + transport, account, emails, name, bookId, useWebSocket, +}): Promise { + const card = buildContactCard({ name, emails, bookId }); const result = await callJmap(transport, { using: [JMAP_CAPS.CORE, JMAP_CAPS.CONTACTS], methodCalls: [[ diff --git a/src/sync/backends/jmap/outbox.ts b/src/sync/backends/jmap/outbox.ts index 8e03e72..bd4896d 100644 --- a/src/sync/backends/jmap/outbox.ts +++ b/src/sync/backends/jmap/outbox.ts @@ -48,7 +48,7 @@ import { JMAP_CAPS } from './transport'; import { callJmap, pickResponse } from './invoke'; import { persistEmails, EMAIL_LIST_PROPERTIES } from './messages'; import { - createTrustedContactCard, + createTrustedContactCards, createContactCard, updateContactCard, deleteContactCard, @@ -173,29 +173,32 @@ async function runSetKeywords({ transport, account, handlers, row, request, useW } /** - * Whitelist a sender: add the message's From address to the trusted- + * Whitelist one or more senders: add each From address to the trusted- * senders address book so Stalwart delivers future authenticated mail - * from that address to the Inbox (trustContacts / card_is_ham). The - * visible rescue of the current message (remove $junk / add $notjunk - * and move Junk → Inbox) is queued separately by the store as - * setKeywords + moveToFolders rows, so this row only owns the contact - * write. request shape: { email, name? }. + * from those addresses to the Inbox (trustContacts / card_is_ham). The + * visible rescue of the messages (remove $junk / add $notjunk and move + * Junk → Inbox) is queued separately by the store as setKeywords + + * moveToFolders rows, so this row only owns the contact writes. request + * shape: { email, name? } for one sender, or { senders: [{ email, name? }, + * ...] } for a bulk whitelist. createTrustedContactCards batches the + * trust writes (one existence query, one book lookup, one multi-create) + * and is idempotent per address, so a retry after a partial failure + * converges. */ async function runWhitelistSender({ transport, account, handlers, request, useWebSocket }) { - const result = await createTrustedContactCard({ - transport, - account, - email: request?.email, - name: request?.name, - useWebSocket, + const senders = Array.isArray(request?.senders) + ? request.senders + : [{ email: request?.email, name: request?.name }]; + const result = await createTrustedContactCards({ + transport, account, senders, useWebSocket, }); if (!result.ok) { return { ok: false, error: result.error ?? { type: 'serverFail' } }; } - // Pull the new card (and its book) into the local cache so it shows - // up in the contacts view without waiting for a StateChange push. - // Best-effort: the trust already took effect server-side, so a - // reconcile failure must not fail the mutation. + // Pull the new cards (and their book) into the local cache once, after + // all trusted cards exist, so they show up in the contacts view without + // waiting for a StateChange push. Best-effort: the trust already took + // effect server-side, so a reconcile failure must not fail the mutation. await reconcileContactsQuietly({ transport, account, handlers, useWebSocket }); return { ok: true }; } diff --git a/tests/e2e/contacts-junk.spec.js b/tests/e2e/contacts-junk.spec.js index c4b15d7..21a2a24 100644 --- a/tests/e2e/contacts-junk.spec.js +++ b/tests/e2e/contacts-junk.spec.js @@ -181,15 +181,15 @@ test.describe('Contacts + Junk whitelist e2e', () => { return keywords.$notjunk === true ? 'notjunk' : JSON.stringify(keywords); }, { timeout: 30_000, message: 'server should mark the message $notjunk and clear $junk' }).toBe('notjunk'); - // Local cache: the message left the Junk view and entered the Inbox - // view (window.__repo, not the worker read-through). + // Local cache: the message left the Junk view (window.__repo, not the + // worker read-through). The destination Inbox window is invalidated and + // re-fetched on open rather than proactively materialized while another + // folder is in view, so — like delete-message.spec / bulk-delete.spec — + // we assert removal from the source cache here and the destination on + // the server (above), not presence in the Inbox view cache. const junkCache = await readViewCacheForFolderRole(page, 'junk'); expect(junkCache, 'local Junk cache should be reachable via window.__repo').not.toBeNull(); expect(junkCache.remoteIds, 'remote id should be gone from the Junk cache').not.toContain(createdId); - await expect.poll(async () => { - const inboxCache = await readViewCacheForFolderRole(page, 'inbox'); - return inboxCache?.remoteIds?.includes(createdId) ?? false; - }, { timeout: 30_000, message: 'whitelisted message should appear in the Inbox cache' }).toBe(true); // Local cache: the trusted sender appears in the contacts cache. await expect.poll(async () => { @@ -205,6 +205,122 @@ test.describe('Contacts + Junk whitelist e2e', () => { } }); + test('Junk multi-select "Not junk" whitelists every unique sender and moves the batch to the Inbox', async ({ sharedPage: page }, testInfo) => { + const jmap = await connectJmap(); + const mailboxes = await listMailboxes(jmap); + const inbox = mailboxByRole(mailboxes, 'inbox'); + const trash = mailboxByRole(mailboxes, 'trash'); + const junk = await ensureJunkMailbox(jmap); + + const ts = Date.now(); + // Two distinct senders, one of them on two messages, so the bulk + // path's per-address de-duplication is exercised (3 messages → 2 + // trusted cards). + const senderA = `bulk-a-${ts}@promo-e2e.example`; + const senderB = `bulk-b-${ts}@promo-e2e.example`; + const cases = [ + { subject: `${JUNK_SUBJECT_PREFIX} bulk ${ts} a1`, sender: senderA }, + { subject: `${JUNK_SUBJECT_PREFIX} bulk ${ts} a2`, sender: senderA }, + { subject: `${JUNK_SUBJECT_PREFIX} bulk ${ts} b1`, sender: senderB }, + ]; + const createdIds = []; + try { + for (const { subject, sender } of cases) { + const id = await createEmailInMailbox(jmap, { + mailboxId: junk.id, + fromEmail: sender, + subject, + keywords: { $junk: true }, + }); + createdIds.push(id); + } + + await clickFolder(page, 'Junk'); + for (const { subject } of cases) { + await expectRowSoon(page, subject); + } + + // Multi-select all three via the checkbox column (no preview open). + for (const { subject } of cases) { + await page.locator('.msg-list__items > li') + .filter({ hasText: subject }) + .first() + .locator('.msg-list__check input') + .click(); + } + await expect(page.locator('.msg-list__count')) + .toHaveText(/^3 selected/, { timeout: 5_000 }); + + // The bulk "Not junk" action is only present in the Junk folder. + await page.locator('.message-view__bulk-actions [title="Whitelist senders and move to Inbox"]').click(); + + // Success toast names the two unique senders. + await expect(page.locator('.store-error-toast__item--success')) + .toContainText(/whitelisted 2 senders/i, { timeout: 30_000 }); + + // Every selected message leaves the Junk list. + for (const { subject } of cases) { + await expect.poll( + async () => page.locator('.msg-list__item').filter({ hasText: subject }).count(), + { timeout: 30_000, message: `whitelisted bulk row "${subject}" should leave Junk` }, + ).toBe(0); + } + + await waitForPendingMutations(page); + + // Server: each message is now in the Inbox and marked $notjunk. + for (const id of createdIds) { + await expect.poll(async () => { + const ids = await getEmailMailboxIds(jmap, id); + if (!ids) return 'missing'; + if (ids[inbox.id] === true && ids[junk.id] !== true) return 'inbox'; + if (ids[junk.id] === true) return 'junk'; + return JSON.stringify(ids); + }, { timeout: 30_000, message: `server should report ${id} in Inbox` }).toBe('inbox'); + await expect.poll(async () => { + const keywords = await getEmailKeywords(jmap, id); + if (!keywords) return 'missing'; + if (keywords.$junk) return 'still-junk'; + return keywords.$notjunk === true ? 'notjunk' : JSON.stringify(keywords); + }, { timeout: 30_000, message: `server should mark ${id} $notjunk` }).toBe('notjunk'); + } + + // Server: both unique senders are trusted, and senderA — used by + // two messages — produced a single card (de-duplicated). + await expect.poll(async () => { + const cards = await listCards(jmap); + return findCardByEmail(cards, senderA) != null && findCardByEmail(cards, senderB) != null; + }, { timeout: 30_000, message: 'both bulk senders should have a trusted ContactCard' }).toBe(true); + const cardsForA = (await listCards(jmap)) + .filter((c) => cardEmails(c).some((a) => a.toLowerCase() === senderA.toLowerCase())); + expect(cardsForA, 'the repeated sender should map to a single trusted card').toHaveLength(1); + + // Local cache: the messages left the Junk view (window.__repo). As in + // the single-message test, the destination Inbox window is invalidated + // and re-fetched on open, so we assert source-cache removal here and + // the Inbox destination on the server (above), not presence in the + // Inbox view cache. + const junkCache = await readViewCacheForFolderRole(page, 'junk'); + expect(junkCache, 'local Junk cache should be reachable via window.__repo').not.toBeNull(); + for (const id of createdIds) { + expect(junkCache.remoteIds, `${id} should be gone from the Junk cache`).not.toContain(id); + } + await expect.poll(async () => { + const cached = await readContactsCache(page); + const emails = new Set((cached ?? []).map((c) => (c.email ?? '').toLowerCase())); + return emails.has(senderA.toLowerCase()) && emails.has(senderB.toLowerCase()); + }, { timeout: 30_000, message: 'both trusted senders should land in the contacts cache' }).toBe(true); + } finally { + await attachConsoleTail(testInfo, consoleLinesFor(page)); + await destroyTestCards(jmap); + if (trash) { + for (const id of createdIds) { + await cleanupEmail(jmap, id, trash.id).catch(() => {}); + } + } + } + }); + test('Contacts add (multi-email) → edit → remove round-trips through the UI and server', async ({ sharedPage: page }, testInfo) => { const jmap = await connectJmap(); const ts = Date.now(); diff --git a/tests/unit/stores/mail-store.test.ts b/tests/unit/stores/mail-store.test.ts index c9e2f9f..c250b21 100644 --- a/tests/unit/stores/mail-store.test.ts +++ b/tests/unit/stores/mail-store.test.ts @@ -25,6 +25,7 @@ import { __resetRepositoryForTests, } from '../../../src/composables/useRepository'; import { TABLE_FAMILIES } from '../../../src/db/protocol'; +import { MUTATION_TYPE } from '../../../src/constants/states'; function makeFolder(id, overrides = {}) { return { @@ -1135,6 +1136,69 @@ describe('selectMessage marks unread as seen via the auto-drained outbox', () => }); }); +describe('whitelistSenders (bulk Not junk)', () => { + it('de-dupes senders into one trust mutation, batches the keyword rescue, and moves the batch', async () => { + const inbox = makeFolder(1, { role: 'inbox', may_add_items: 1 }); + const junk = makeFolder(2, { role: 'junk', may_remove_items: 1, total_emails: 3 }); + const rows = [ + makeRow(10, { from_text: 'Alice ', keywords_json: '{"$junk":true}' }), + makeRow(11, { from_text: 'a@x.com', keywords_json: '{"$junk":true}' }), + makeRow(12, { from_text: 'Bob ', keywords_json: '{"$junk":true}' }), + ]; + const { mailStore, repo } = await setupStore({ + folders: [inbox, junk], + views: { 1: { rows: [], total: 0 }, 2: { rows, total: 3 } }, + }); + await flush(); + + mailStore.selectFolder(junk.id); + await flush(); + expect(mailStore.currentFolder?.role).toBe('junk'); + + const insertCalls = []; + repo.insertPendingMutation = async (input) => { + insertCalls.push(input); + return { id: 100 + insertCalls.length }; + }; + let keywordBatch = null; + repo.replaceMessageKeywordsMany = async (items) => { + keywordBatch = items; + return { ok: true, applied: items.length }; + }; + + const result = await mailStore.whitelistSenders([10, 11, 12]); + await flush(); + + expect(result.succeeded).toBe(3); + + const byType = (type) => insertCalls.filter((c) => c.mutationType === type); + + // One trust mutation carrying the two unique senders (deduped). + const trust = byType(MUTATION_TYPE.WHITELIST_SENDER); + expect(trust).toHaveLength(1); + expect(trust[0].targetMessageId).toBeNull(); + expect(JSON.parse(trust[0].requestJson).senders.map((s) => s.email)) + .toEqual(['a@x.com', 'b@y.com']); + + // One batched keyword rescue across all three messages. + expect(keywordBatch).toHaveLength(3); + const setKw = byType(MUTATION_TYPE.SET_KEYWORDS); + expect(setKw).toHaveLength(1); + expect(JSON.parse(setKw[0].requestJson)).toMatchObject({ + messageIds: [10, 11, 12], + add: ['$notjunk'], + remove: ['$junk'], + }); + + // One move into the Inbox. + expect(byType(MUTATION_TYPE.MOVE_TO_FOLDERS)).toHaveLength(1); + + // Rows leave the Junk view and a success notice names the senders. + expect(mailStore.messages.map((r) => r?.id)).toEqual([]); + expect(mailStore.notice).toBe('Whitelisted 2 senders — moved 3 messages to Inbox'); + }); +}); + describe('successor selection after removal', () => { it('selects the next message after deleting the previewed row', async () => { const inbox = makeFolder(1, { total_emails: 3 }); diff --git a/tests/unit/sync/jmap-contacts.test.ts b/tests/unit/sync/jmap-contacts.test.ts index 73c4388..b268553 100644 --- a/tests/unit/sync/jmap-contacts.test.ts +++ b/tests/unit/sync/jmap-contacts.test.ts @@ -9,11 +9,18 @@ import { syncContacts, syncContactCardChanges, createContactCard, + createTrustedContactCards, updateContactCard, deleteContactCard, } from '../../../src/sync/backends/jmap/contacts'; import { MockTransport } from './_mock-transport'; +function countMethod(transport, name) { + return transport.requests.filter( + (r) => r.methodCalls.some(([m]) => m === name), + ).length; +} + let engine; let handlers; let account; @@ -217,6 +224,95 @@ describe('createContactCard', () => { }); }); +describe('createTrustedContactCards', () => { + function setup({ existingIds = [], existingCards = [] } = {}) { + const transport = new MockTransport(); + transport.handle('ContactCard/query', () => ({ ids: existingIds, total: existingIds.length })); + transport.handle('ContactCard/get', () => ({ list: existingCards })); + transport.handle('AddressBook/get', () => ({ + list: [{ id: 'book-trusted', name: 'Trusted senders', isDefault: false }], + })); + let created: any = null; + transport.handle('ContactCard/set', (params) => { + created = params.create; + const out: Record = {}; + for (const key of Object.keys(params.create ?? {})) out[key] = { id: `new-${key}` }; + return { created: out }; + }); + return { transport, getCreated: () => created }; + } + + it('trusts every unique sender in one ContactCard/set and de-dupes by address', async () => { + const { transport, getCreated } = setup(); + const result = await createTrustedContactCards({ + transport, + account, + senders: [ + { email: 'a@x.com', name: 'Alice' }, + { email: 'A@X.com', name: 'Alice dup' }, + { email: 'b@y.com', name: 'Bob' }, + ], + }); + expect(result.ok).toBe(true); + expect(result.created).toBe(2); + + const created = getCreated(); + expect(Object.values(created).map((c: any) => c.emails.e1.address)) + .toEqual(['a@x.com', 'b@y.com']); + Object.values(created).forEach((c: any) => { + expect(c.addressBookIds).toEqual({ 'book-trusted': true }); + }); + + // Proper batch: one book lookup and one create regardless of N. + expect(countMethod(transport, 'AddressBook/get')).toBe(1); + expect(countMethod(transport, 'ContactCard/set')).toBe(1); + expect(countMethod(transport, 'ContactCard/query')).toBe(1); + }); + + it('skips addresses that already have a card and only creates the rest', async () => { + const { transport, getCreated } = setup({ + existingIds: ['existing-1'], + existingCards: [{ id: 'existing-1', emails: { e1: { address: 'a@x.com' } } }], + }); + const result = await createTrustedContactCards({ + transport, + account, + senders: [{ email: 'a@x.com', name: 'Alice' }, { email: 'b@y.com', name: 'Bob' }], + }); + expect(result.ok).toBe(true); + expect(result.created).toBe(1); + expect(Object.values(getCreated()).map((c: any) => c.emails.e1.address)).toEqual(['b@y.com']); + }); + + it('reports alreadyTrusted and issues no create when every sender exists', async () => { + const { transport } = setup({ + existingIds: ['e1', 'e2'], + existingCards: [ + { id: 'e1', emails: { e1: { address: 'a@x.com' } } }, + { id: 'e2', emails: { e1: { address: 'b@y.com' } } }, + ], + }); + const result = await createTrustedContactCards({ + transport, + account, + senders: [{ email: 'a@x.com' }, { email: 'b@y.com' }], + }); + expect(result).toEqual({ ok: true, created: 0, alreadyTrusted: true }); + expect(countMethod(transport, 'ContactCard/set')).toBe(0); + expect(countMethod(transport, 'AddressBook/get')).toBe(0); + }); + + it('fails without touching the server when no valid sender is provided', async () => { + const transport = new MockTransport(); + const result = await createTrustedContactCards({ + transport, account, senders: [{ email: ' ' }, { email: null }], + }); + expect(result.ok).toBe(false); + expect(result.error.type).toBe('invalidArguments'); + expect(transport.requests).toHaveLength(0); + }); +}); + describe('updateContactCard', () => { // A card with extra fields the editor never shows, plus per-email // metadata, to prove the merge never silently erases anything. From b946a024d7543528759740dc3df1bb88ba15d968 Mon Sep 17 00:00:00 2001 From: Andrei Hajdukewycz Date: Tue, 16 Jun 2026 05:05:36 +0000 Subject: [PATCH 2/2] Require batched mail operations in the constitution. Mutation Pipeline (IV) now requires every mail operation to support both single and batched messages (the single action being the N=1 case) and a batch to be a real batch at every layer: one multi-object protocol call per chunk, batched SQL, and a coalesced paint. Also extends the MVP spec's Junk-whitelist requirement to cover the multi-selected batch. --- .specify/memory/constitution.md | 23 ++++++++++++++++++++--- specs/001-mvp-scope/spec.md | 2 +- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/.specify/memory/constitution.md b/.specify/memory/constitution.md index 42a9821..18d4d36 100644 --- a/.specify/memory/constitution.md +++ b/.specify/memory/constitution.md @@ -55,8 +55,17 @@ Stormbox-specific backend. an asynchronous push is not sufficient. - Ordinary delete moves a message to Trash. Permanent destroy is reserved for messages already in Trash or explicit destroy flows. -- Bulk applies shall batch SQL and coalesce per-row UI refreshes into - one paint. +- Mail operations (triage and message actions) shall support both a + single message and a multi-selected batch; the single-message action + is the N=1 case of the batched path, not a separate implementation. +- A batch shall be a real batch at every layer, not a per-item loop: + - Protocol: dispatch the chunk in as few calls as the server API + allows — a single multi-object request (e.g. JMAP `Email/set` / + `ContactCard/set` with multiple ids or a creation map, or query/get + back-references), resolving shared prerequisites once rather than + repeating the round trip per item. + - Storage: batch the chunk's SQL writes. + - UI: coalesce the per-row refreshes into a single paint. ### V. Incremental Sync @@ -134,4 +143,12 @@ architectural constraints. Feature specs, plans, and tasks must call out any conflict before implementation begins. Amendments require an update to this file with a brief reason. -**Version**: 1.1.0 | **Ratified**: 2026-05-21 | **Last Amended**: 2026-05-22 +**Version**: 1.3.0 | **Ratified**: 2026-05-21 | **Last Amended**: 2026-06-15 + + diff --git a/specs/001-mvp-scope/spec.md b/specs/001-mvp-scope/spec.md index 0d88952..f70ba2a 100644 --- a/specs/001-mvp-scope/spec.md +++ b/specs/001-mvp-scope/spec.md @@ -86,7 +86,7 @@ capability. | R-3.13 🟧 Planned | The system shall provide an undo/redo queue for message triage operations including archive, move, delete (to Trash), and mark read/unread. The user shall be able to reverse the most recent action via Ctrl/Cmd+Z and reapply it via Ctrl/Cmd+Shift+Z (or Ctrl/Cmd+Y), matching Thunderbird-compatible behavior. The queue shall preserve a bounded history of recent operations within the active session, group bulk actions on multiple messages into a single undoable entry, and surface a transient confirmation (e.g. toast) after each action with an undo affordance. Permanent destroy (Shift+Delete and Trash purge) shall remain non-undoable. The queue shall be cleared on sign-out and is not required to survive page reloads. | | R-3.14 🟩 Done | When the user issues a move or delete (move-to-Trash or permanent destroy) covering more than a configurable batch size (default 200 messages), the system shall split the dispatch into sequential JMAP `Email/set` chunks no larger than that batch size, display a modal progress indicator that names the operation and shows messages-completed / total, and block other user input until the operation finishes. Each chunk shall be its own `pending_mutations` row so the outbox apply step still keeps the local cache authoritative per chunk. On a chunk failure the system shall stop further chunks, leave already-succeeded chunks applied, surface an error that distinguishes the partial outcome (e.g. "Could not move message (requestTooLarge) (400 of 536 succeeded).") derived from the JMAP method-level error type when the server returns one, and clear the progress indicator. The batch size shall be a single source-level constant so it can be tuned without schema or API changes. | | R-3.15 🟩 Done | The active row shall be tracked as a single stable message-id cursor shared by every navigation path — arrow keys on the focused list, the Thunderbird shortcuts of R-3.7, row clicks, and the automatic preview advance of R-3.11. The cursor shall coincide with the previewed message on plain navigation and clicks; during a Shift+Arrow range extension (R-3.6) the cursor shall advance to the range's leading edge while the previewed message stays put. When the cursor moves, the system shall scroll the virtualized message list (R-2.2) so the cursor row is brought into view with minimal movement (no scroll when it is already fully visible), because an off-screen row in the virtualized list is not present in the DOM and cannot be revealed by element-level scrolling. The message list shall expose listbox semantics for assistive technology: a listbox container, option rows carrying their selected state, and an active-descendant reference that tracks the cursor row kept in view. | -| R-3.16 🟩 Done | When the user opens a message in the Junk folder, the system shall offer a "Not junk" action that whitelists the sender and rescues the message. Whitelisting shall add the sender's address to a dedicated "Trusted senders" address book as a `ContactCard` so the server's contact trust delivers future authenticated mail from that address to the Inbox; rescuing shall clear the `$junk` keyword, set `$notjunk`, and move the message from Junk to the Inbox. The action shall be offered only from the Junk folder. The system shall confirm success only when the sender was actually trusted; when the message moved but the trust write did not apply, the system shall surface that partial outcome rather than reporting that the sender was whitelisted. | +| R-3.16 🟩 Done | When the user opens a message in the Junk folder, the system shall offer a "Not junk" action that whitelists the sender and rescues the message; the same action shall be offered for a multi-selected batch in the Junk folder, whitelisting each unique sender once (de-duplicated by address) and rescuing every selected message. Whitelisting shall add each sender's address to a dedicated "Trusted senders" address book as a `ContactCard` so the server's contact trust delivers future authenticated mail from that address to the Inbox; rescuing shall clear the `$junk` keyword, set `$notjunk`, and move the affected messages from Junk to the Inbox. The action shall be offered only from the Junk folder. The system shall confirm success only when the senders were actually trusted; when the messages moved but the trust write did not apply, the system shall surface that partial outcome rather than reporting that the senders were whitelisted. | ### 4. Compose and send