From a5c5fa6669508a5c94288c1ac4872ba427d2d169 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 19 Mar 2026 18:00:09 +0100 Subject: [PATCH] fix: improve mailbox role management by ensuring roles are cleared from all mailboxes when reassigning --- components/settings/folder-settings.tsx | 43 ++++++++++++++-------- lib/utils.ts | 16 +++++--- stores/__tests__/folder-management.test.ts | 28 ++++++++++++++ stores/email-store.ts | 8 ++-- 4 files changed, 70 insertions(+), 25 deletions(-) diff --git a/components/settings/folder-settings.tsx b/components/settings/folder-settings.tsx index d1db8945..b9855902 100644 --- a/components/settings/folder-settings.tsx +++ b/components/settings/folder-settings.tsx @@ -490,21 +490,34 @@ export function FolderSettings() { {/* Standard Folder Roles — advanced section */} - {STANDARD_ROLES.map((role) => ( - - handleRoleChange(role, value)} + options={[ + { value: '', label: t('role_none') }, + ...ownMailboxes.map(mb => ({ + value: mb.id, + label: (nameCounts.get(mb.name) || 0) > 1 + ? `${getParentPath(mb)}${mb.name} (${mb.id.slice(-6)})` + : mb.name, + })), + ]} + /> + + ); + })} ); diff --git a/lib/utils.ts b/lib/utils.ts index 3daf3252..0c007f2a 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -97,17 +97,19 @@ const ROLE_PRIORITY: Record = { // Deduplicate mailboxes (e.g., "Sent" vs "Sent Mail") function deduplicateMailboxes(mailboxes: Mailbox[]): Mailbox[] { - const roleMap = new Map(); const result: Mailbox[] = []; - // First pass: collect mailboxes with roles + // Group role mailboxes by account so deduplication is scoped per-account + const rolesByAccount = new Map(); mailboxes.forEach(mb => { if (mb.role) { - roleMap.set(mb.role, mb); + const key = mb.accountId || ''; + if (!rolesByAccount.has(key)) rolesByAccount.set(key, []); + rolesByAccount.get(key)!.push(mb); } }); - // Second pass: filter out duplicates + // Filter out duplicates scoped to the same account mailboxes.forEach(mb => { // If this mailbox has a role, always keep it if (mb.role) { @@ -115,9 +117,11 @@ function deduplicateMailboxes(mailboxes: Mailbox[]): Mailbox[] { return; } - // Check if this is a duplicate of a role-based mailbox + // Check if this is a duplicate of a role-based mailbox in the SAME account + const accountKey = mb.accountId || ''; + const accountRoles = rolesByAccount.get(accountKey) || []; const lowerName = mb.name.toLowerCase(); - const isDuplicate = Array.from(roleMap.values()).some(roleMb => { + const isDuplicate = accountRoles.some(roleMb => { const roleLowerName = roleMb.name.toLowerCase(); // Check for common duplicates: "Sent Mail" vs "Sent", etc. return lowerName.includes(roleLowerName) || roleLowerName.includes(lowerName); diff --git a/stores/__tests__/folder-management.test.ts b/stores/__tests__/folder-management.test.ts index 4c030f40..72457343 100644 --- a/stores/__tests__/folder-management.test.ts +++ b/stores/__tests__/folder-management.test.ts @@ -226,6 +226,34 @@ describe('email-store folder management', () => { expect(client.updateMailbox).toHaveBeenCalledWith('trash-1', { role: 'trash' }); }); + it('should clear role from ALL mailboxes with that role when reassigning', async () => { + // Simulate server anomaly: two mailboxes with role "trash" + const extraTrash = makeMailbox({ id: 'trash-2', name: 'Deleted Items', role: 'trash' }); + useEmailStore.setState({ + mailboxes: [inbox, sent, trash, custom, extraTrash], + }); + + const newMailboxes = [inbox, sent, custom, + makeMailbox({ id: 'trash-1', name: 'Trash', role: undefined }), + makeMailbox({ id: 'trash-2', name: 'Deleted Items', role: undefined }), + ]; + // custom-1 gets the trash role + newMailboxes[2] = { ...newMailboxes[2], role: 'trash' }; + + const client = makeMockClient({ + getAllMailboxes: vi.fn().mockResolvedValue(newMailboxes), + }); + + await useEmailStore.getState().setMailboxRole(client, 'custom-1', 'trash'); + + // Should clear trash role from BOTH trash-1 and trash-2 + expect(client.updateMailbox).toHaveBeenCalledWith('trash-1', { role: null }); + expect(client.updateMailbox).toHaveBeenCalledWith('trash-2', { role: null }); + // Then set trash role on custom-1 + expect(client.updateMailbox).toHaveBeenCalledWith('custom-1', { role: 'trash' }); + expect(client.updateMailbox).toHaveBeenCalledTimes(3); + }); + it('should set error on failure', async () => { const client = makeMockClient({ updateMailbox: vi.fn().mockRejectedValue(new Error('Role update failed')), diff --git a/stores/email-store.ts b/stores/email-store.ts index 33ed6d8e..1822bf9e 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -1298,11 +1298,11 @@ export const useEmailStore = create((set, get) => ({ setMailboxRole: async (client, mailboxId, role) => { try { - // If assigning a role, first clear that role from any other mailbox + // If assigning a role, first clear that role from ALL other mailboxes that have it if (role) { - const existingMailbox = get().mailboxes.find(mb => mb.role === role && !mb.isShared); - if (existingMailbox && existingMailbox.id !== mailboxId) { - await client.updateMailbox(existingMailbox.id, { role: null }); + const existingMailboxes = get().mailboxes.filter(mb => mb.role === role && !mb.isShared && mb.id !== mailboxId); + for (const existing of existingMailboxes) { + await client.updateMailbox(existing.id, { role: null }); } } await client.updateMailbox(mailboxId, { role });