fix: make undo after a star take back the star, not the tag set before it

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<void>) => {
+    if (onlineRun) await onlineRun();
+    return { queued: false };
+  });
+  return {
+    applyOrQueueBatch,
+    applyOrQueue: async (op: unknown, onlineRun?: () => Promise<void>) => applyOrQueueBatch([op], onlineRun),
+    useOutboxStore: {
+      getState: () => ({ entries: [], count: () => 0, setAccount: vi.fn(async () => undefined), flush: vi.fn() }),
+    },
+  };
+});
+
+vi.mock('../settings-store', () => {
+  const settings: Record<string, unknown> = { 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<typeof vi.fn>;
+const mockMoveEmail = emailApi.moveEmail as ReturnType<typeof vi.fn>;
+const mockRestore = emailApi.restoreEmailMailboxes as ReturnType<typeof vi.fn>;
+const mockMarkAsSpam = emailApi.markAsSpam as ReturnType<typeof vi.fn>;
+const mockUndoSpam = emailApi.undoSpam as ReturnType<typeof vi.fn>;
+const mockArchive = emailApi.archiveEmails as ReturnType<typeof vi.fn>;
+const mockPatchKeywords = emailApi.patchKeywordsForEmails as ReturnType<typeof vi.fn>;
+const mockApplyOrQueueBatch = applyOrQueueBatch as unknown as ReturnType<typeof vi.fn>;
+
+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<typeof vi.fn>;
+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<void>((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 });
+  });
+});

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<undefined>((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 } });

src/stores/email-store.ts

@@ -2160,6 +2160,10 @@ export const useEmailStore = create<EmailState>()(
     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);

src/stores/outbox-store.ts

@@ -394,6 +394,8 @@ export const useOutboxStore = create<OutboxState>((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<OutboxState>((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<OutboxState>((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<OutboxState>((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 {