diff --git a/src/stores/__tests__/email-store-keyword-undo.test.ts b/src/stores/__tests__/email-store-keyword-undo.test.ts new file mode 100644 index 00000000..eb6e8015 --- /dev/null +++ b/src/stores/__tests__/email-store-keyword-undo.test.ts @@ -0,0 +1,163 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// Actions the viewer runs on the message it shows, which may live in another +// account than the open folder (unified inbox, notification, deep link). +// Stalwart ids are only unique per account, so the fixtures reuse the same +// message id ("e1") and raw folder ids ("a" = Inbox, "t" = Trash) in the +// user's own account and in the group account "grp-1". + +vi.mock('../../api/email', () => ({ + getMailboxes: vi.fn(), + getMailboxesWithState: vi.fn(async () => ({ list: [], state: 'mb-state-0' })), + getSharedMailboxes: vi.fn(async () => []), + getMailboxesByIds: vi.fn(async () => ({ list: [], state: 'mb-state-0' })), + getMailboxChanges: vi.fn(async () => null), + queryEmails: vi.fn(), + getEmailQueryChanges: vi.fn(async () => null), + getEmailListDelta: vi.fn(async () => ({ queryChanges: null, changes: null, added: [], addedFetched: false, threads: [] })), + getEmails: vi.fn(), + getEmailsWithState: vi.fn(async () => ({ list: [], state: 'em-state-0' })), + getEmailChanges: vi.fn(async () => null), + getFullEmail: vi.fn(), + importEmailBlob: vi.fn(), + patchKeywordsForEmails: vi.fn(), + patchKeywordsPerEmail: vi.fn(), + moveEmail: vi.fn(), + moveEmails: vi.fn(), + archiveEmails: vi.fn(), + restoreEmailMailboxes: vi.fn(), + setEmailMailboxes: vi.fn(), + destroyEmails: vi.fn(), + deleteEmail: vi.fn(), + deleteEmails: vi.fn(), + searchEmails: vi.fn(), + markAsSpam: vi.fn(), + undoSpam: vi.fn(), + unprefixMailboxId: (id: string, accountId?: string) => + (accountId && id.startsWith(`${accountId}:`) ? id.slice(accountId.length + 1) : id), +})); + +vi.mock('../locale-store', () => ({ + t: (_key: string, fallback?: string) => fallback ?? _key, + useLocaleStore: { getState: () => ({ locale: 'en', t: (_k: string, f?: string) => f ?? _k }) }, +})); + +// Online with nothing queued: run the online runner straight away. +vi.mock('../outbox-store', () => { + const applyOrQueueBatch = vi.fn(async (_ops: unknown[], onlineRun?: () => Promise) => { + if (onlineRun) await onlineRun(); + return { queued: false }; + }); + return { + applyOrQueueBatch, + applyOrQueue: async (op: unknown, onlineRun?: () => Promise) => applyOrQueueBatch([op], onlineRun), + useOutboxStore: { + getState: () => ({ entries: [], count: () => 0, setAccount: vi.fn(async () => undefined), flush: vi.fn() }), + }, + }; +}); + +vi.mock('../settings-store', () => { + const settings: Record = { archiveMode: 'single', deleteAction: 'trash' }; + return { + useSettingsStore: { + getState: () => ({ + ...settings, + updateSetting: (key: string, value: unknown) => { settings[key] = value; }, + }), + }, + }; +}); + +vi.mock('../offline-cache-store', () => ({ + useOfflineCacheStore: { + getState: () => ({ + has: () => false, + get: vi.fn(async () => null), + put: vi.fn(async () => undefined), + patch: vi.fn(async () => undefined), + remove: vi.fn(async () => undefined), + }), + }, +})); + +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + isConnected: true, + accountId: 'acc-1', + username: 'test@example.com', + serverUrl: 'https://mail.example.com', + getMaxObjectsInGet: () => 500, + getMaxCallsInRequest: () => 16, + getSharedMailAccounts: () => [{ id: 'grp-1', name: 'Support' }], + request: vi.fn(), + }, +})); + +import * as emailApi from '../../api/email'; +import { applyOrQueueBatch } from '../outbox-store'; +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; +const mockArchive = emailApi.archiveEmails as ReturnType; +const mockPatchKeywords = emailApi.patchKeywordsForEmails as ReturnType; +const mockApplyOrQueueBatch = applyOrQueueBatch as unknown as ReturnType; + +function own(id: string, role: string): Mailbox { + return { id, name: role, role, accountId: 'acc-1', isShared: false } as Mailbox; +} +function team(rawId: string, role: string): Mailbox { + return { id: `grp-1:${rawId}`, originalId: rawId, name: role, role, accountId: 'grp-1', isShared: true } as Mailbox; +} + +const MAILBOXES = [ + own('a', 'inbox'), own('t', 'trash'), own('x', 'archive'), own('j', 'junk'), + team('a', 'inbox'), team('t', 'trash'), team('x', 'archive'), team('j', 'junk'), +]; + +const mockPatchPerEmail = emailApi.patchKeywordsPerEmail as ReturnType; +const ROW = { id: 'e1', subject: 'Hello', keywords: { $seen: true }, mailboxIds: { a: true } } as unknown as Email; + +beforeEach(() => { + vi.clearAllMocks(); + useEmailStore.getState().reset(); +}); + +describe('keyword undo', () => { + it('undoes a star made after a tag', async () => { + useEmailStore.setState({ mailboxes: MAILBOXES, currentMailboxId: 'a', emails: [ROW] }); + + await useEmailStore.getState().setKeywordForEmails(['e1'], '$label:orange', true); + await useEmailStore.getState().setKeywordForEmails(['e1'], '$flagged', true); + await useEmailStore.getState().undoLast(); + + expect(mockPatchPerEmail).toHaveBeenLastCalledWith([{ id: 'e1', patch: { $flagged: null } }], undefined); + expect(useEmailStore.getState().emails[0].keywords).toEqual({ $seen: true, '$label:orange': true }); + }); + + it("does not offer the tag's Undo while the star after it is on its way", async () => { + useEmailStore.setState({ mailboxes: MAILBOXES, currentMailboxId: 'a', emails: [ROW] }); + await useEmailStore.getState().setKeywordForEmails(['e1'], '$label:orange', true); + expect(useEmailStore.getState().pendingUndo?.keywordPatch).toEqual({ '$label:orange': true }); + + let release!: () => void; + mockPatchKeywords.mockImplementationOnce(() => new Promise((r) => { release = () => r(); })); + const starring = useEmailStore.getState().setKeywordForEmails(['e1'], '$flagged', true); + // Undo tapped on the bar still up from the tag. + await useEmailStore.getState().undoLast(); + expect(mockPatchPerEmail).not.toHaveBeenCalled(); + + release(); + await starring; + await useEmailStore.getState().undoLast(); + expect(mockPatchPerEmail).toHaveBeenCalledTimes(1); + expect(mockPatchPerEmail).toHaveBeenCalledWith([{ id: 'e1', patch: { $flagged: null } }], undefined); + expect(useEmailStore.getState().emails[0].keywords).toEqual({ $seen: true, '$label:orange': true }); + }); +}); diff --git a/src/stores/__tests__/outbox-store.test.ts b/src/stores/__tests__/outbox-store.test.ts index 2c88d927..609fb5f0 100644 --- a/src/stores/__tests__/outbox-store.test.ts +++ b/src/stores/__tests__/outbox-store.test.ts @@ -155,6 +155,25 @@ describe('flush', () => { expect(useOutboxStore.getState().entries).toHaveLength(0); }); + it('sends an edit merged into an entry while that entry was being sent', async () => { + useNetworkStore.setState({ online: false }); + useOutboxStore.getState().enqueue({ kind: 'keywords', emailId: 'e1', patch: { $flagged: true } }); + useNetworkStore.setState({ online: true }); + + // The star is out when the Undo comes in and coalesces into its entry. + let release!: () => void; + patchKeywordsForEmails.mockImplementationOnce(() => new Promise((r) => { release = () => r(undefined); })); + const flushing = useOutboxStore.getState().flush(); + await vi.waitFor(() => expect(patchKeywordsForEmails).toHaveBeenCalledTimes(1)); + useOutboxStore.getState().enqueue({ kind: 'keywords', emailId: 'e1', patch: { $flagged: null } }); + release(); + await flushing; + + await vi.waitFor(() => expect(patchKeywordsForEmails).toHaveBeenCalledTimes(2)); + expect(patchKeywordsForEmails).toHaveBeenLastCalledWith(['e1'], { $flagged: null }, undefined); + await vi.waitFor(() => expect(useOutboxStore.getState().entries).toHaveLength(0)); + }); + it('does nothing while offline', async () => { useNetworkStore.setState({ online: false }); useOutboxStore.getState().enqueue({ kind: 'keywords', emailId: 'e1', patch: { $seen: true } }); diff --git a/src/stores/email-store.ts b/src/stores/email-store.ts index fccf662f..f59d7c6b 100644 --- a/src/stores/email-store.ts +++ b/src/stores/email-store.ts @@ -2160,6 +2160,10 @@ export const useEmailStore = create()( const state = get(); const { targets, listed } = actionTargets(state, emailIds, viewed); if (targets.length === 0) return; + // While this change is on its way the bar must not still offer the + // previous one's Undo: tapping it right after a star undid the tag set + // before, and the star stayed. + if (!viewed && keywordUndoLabel(token, on, targets.length) && get().pendingUndo) set({ pendingUndo: null }); // One Email/set per account the selection spans (#1082); the viewer's // message goes to the account it names. const groups = viewed ? [{ accountId: viewed.accountId, emails: targets }] : groupByAccount(state, targets); diff --git a/src/stores/outbox-store.ts b/src/stores/outbox-store.ts index 2f03d6de..7f78a33b 100644 --- a/src/stores/outbox-store.ts +++ b/src/stores/outbox-store.ts @@ -394,6 +394,8 @@ export const useOutboxStore = create((set, get) => ({ set({ flushing: true }); let brokeTransient = false; let dropped = false; + // An entry that took on a later edit while its op was out runs again. + let changedInFlight = false; try { // Process oldest-first so dependent moves replay in the order they were // made. We snapshot the order but re-read the live list each iteration, @@ -413,7 +415,9 @@ export const useOutboxStore = create((set, get) => ({ try { await runOp(entry.op); - removeEntry(accountId, entry.id); + // An edit queued meanwhile was merged into this entry (an Undo of + // the star being sent): dropping the entry would lose it. + if (!removeEntry(accountId, entry.id, entry.op)) changedInFlight = true; retryAttempt = 0; } catch (err) { if (get().activeAccountId !== accountId) break; @@ -446,6 +450,7 @@ export const useOutboxStore = create((set, get) => ({ set({ flushing: false }); } if (brokeTransient && get().entries.length > 0) scheduleRetry(); + else if (changedInFlight) void get().flush(); if (dropped) void refreshAfterDrop(); }, @@ -502,12 +507,20 @@ export const useOutboxStore = create((set, get) => ({ // ── Internal helpers that mutate+persist the live list ────────────────────── -function removeEntry(accountId: string, id: string): void { +/** + * Drops a replayed entry. With `sent`, only while the entry still holds that + * op: one an enqueue coalesced into while it was out stays, and false says + * so. + */ +function removeEntry(accountId: string, id: string, sent?: OutboxOp): boolean { const store = useOutboxStore.getState(); - if (store.activeAccountId !== accountId) return; + if (store.activeAccountId !== accountId) return true; + const current = store.entries.find((e) => e.id === id); + if (current && sent && current.op !== sent) return false; const entries = store.entries.filter((e) => e.id !== id); useOutboxStore.setState({ entries }); persist(accountId, entries); + return true; } function moveToFailed(accountId: string, id: string, err: unknown): void {