From dafdda5347e544316380e6e37e20743fc993a3bd Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 01:16:34 +0100 Subject: [PATCH] fix: let search results move into the open folder #1452 --- ...-context-menu-move-to-open-folder.test.tsx | 81 +++++++++++++++++++ .../email/conversation-header-actions.tsx | 4 +- components/email/email-context-menu.tsx | 1 + components/email/email-viewer.tsx | 8 +- components/email/move-to-dialog.tsx | 1 + components/email/mv-list-toolbar.tsx | 1 + lib/__tests__/move-targets.test.ts | 19 +++++ lib/move-targets.ts | 47 ++++++++++- .../email-store-plugin-organize-hooks.test.ts | 17 ++++ stores/email-store.ts | 36 +-------- 10 files changed, 176 insertions(+), 39 deletions(-) create mode 100644 components/email/__tests__/email-context-menu-move-to-open-folder.test.tsx diff --git a/components/email/__tests__/email-context-menu-move-to-open-folder.test.tsx b/components/email/__tests__/email-context-menu-move-to-open-folder.test.tsx new file mode 100644 index 000000000..932a82b11 --- /dev/null +++ b/components/email/__tests__/email-context-menu-move-to-open-folder.test.tsx @@ -0,0 +1,81 @@ +import { render, screen, fireEvent } from '@testing-library/react'; +import { beforeEach, describe, it, expect, vi } from 'vitest'; +import type { Email, Mailbox } from '@/lib/jmap/types'; +import { useEmailStore } from '@/stores/email-store'; +import { DEFAULT_SEARCH_FILTERS } from '@/lib/jmap/search-utils'; + +vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: () => null })); +vi.mock('../rules-menu', () => ({ RulesContextSubMenu: () => null })); +vi.mock('@/hooks/use-copy-link', () => ({ useCopyLink: () => vi.fn() })); + +const { EmailContextMenu } = await import('../email-context-menu'); + +// A search across folders keeps the open folder selected. Its hits from other +// folders can be moved into that folder (#1452); a message already in it can't. + +function mailbox(id: string, extra: Partial = {}): Mailbox { + return { + id, name: id, role: undefined, parentId: undefined, totalEmails: 0, unreadEmails: 0, + myRights: { mayAddItems: true }, ...extra, + } as unknown as Mailbox; +} + +const mailboxes = [mailbox('inbox', { role: 'inbox', name: 'Inbox' }), mailbox('work', { name: 'Work' })]; +const inWork = { id: 'e1', keywords: { $seen: true }, mailboxIds: { work: true } } as unknown as Email; +const inInbox = { id: 'e2', keywords: { $seen: true }, mailboxIds: { inbox: true } } as unknown as Email; + +beforeEach(() => { + useEmailStore.setState({ + mailboxes, + accountMailboxes: {}, + viewingAccountId: null, + selectedMailbox: 'inbox', + selectedKeyword: null, + isUnifiedView: false, + crossView: null, + isScheduledView: false, + searchQuery: 'invoice', + searchFilters: DEFAULT_SEARCH_FILTERS, + searchMailboxId: '', + }); +}); + +function openMoveMenu(props: Partial>) { + const result = render( + {}} + menuRef={{ current: null }} + mailboxes={mailboxes} + selectedMailbox="inbox" + onMoveToMailbox={vi.fn()} + {...props} + />, + ); + fireEvent.mouseEnter(screen.getByTestId('ctx-move-to').parentElement!); + return result; +} + +describe('Move to the open folder from search results', () => { + it('offers the open folder for a hit from another folder, and moves it there', () => { + const onMoveToMailbox = vi.fn(); + openMoveMenu({ onMoveToMailbox }); + fireEvent.click(screen.getByTestId('move-to:inbox')); + expect(onMoveToMailbox).toHaveBeenCalledWith('inbox'); + }); + + it('does not offer the open folder for a message already in it', () => { + openMoveMenu({ email: inInbox }); + expect(screen.queryByTestId('move-to:inbox')).toBeNull(); + expect(screen.getByTestId('move-to:work')).toBeTruthy(); + }); + + it('offers it for a selection when one of the messages is elsewhere', () => { + const onBatchMoveToMailbox = vi.fn(); + openMoveMenu({ isMultiSelect: true, selectedCount: 2, batchEmails: [inInbox, inWork], onBatchMoveToMailbox }); + fireEvent.click(screen.getByTestId('move-to:inbox')); + expect(onBatchMoveToMailbox).toHaveBeenCalledWith('inbox'); + }); +}); diff --git a/components/email/conversation-header-actions.tsx b/components/email/conversation-header-actions.tsx index e36ec9020..6c62cc510 100644 --- a/components/email/conversation-header-actions.tsx +++ b/components/email/conversation-header-actions.tsx @@ -56,8 +56,8 @@ export function ConversationHeaderActions({ email, actions }: { email: Email; ac const ownerAccountId = resolveMoveOwnerAccountId(email, mailboxes, selectedMailbox); const { tree: moveTree, targetIds: moveTargetIds } = useMemo( - () => buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId }), - [mailboxes, selectedMailbox, ownerAccountId], + () => buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId, emails: [email] }), + [mailboxes, selectedMailbox, ownerAccountId, email], ); const canMove = !!onMoveToMailbox && moveTree.length > 0; diff --git a/components/email/email-context-menu.tsx b/components/email/email-context-menu.tsx index dd1c11679..054018ece 100644 --- a/components/email/email-context-menu.tsx +++ b/components/email/email-context-menu.tsx @@ -214,6 +214,7 @@ export function EmailContextMenu({ const { tree: moveTree, targetIds: moveTargetIds } = buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId: resolveMoveOwnerAccountId(email, mailboxes, selectedMailbox), + emails: showBatchActions ? batchEmails : [email], }); // Filter tree to only include branches that contain valid targets diff --git a/components/email/email-viewer.tsx b/components/email/email-viewer.tsx index dae23f296..6e257df6d 100644 --- a/components/email/email-viewer.tsx +++ b/components/email/email-viewer.tsx @@ -1903,8 +1903,12 @@ export function EmailViewer({ // Move-to dropdown: the message's own account first (#1149) const moveOwnerAccountId = resolveMoveOwnerAccountId(email, mailboxes, selectedMailbox); const { tree: moveTree, targetIds: moveTargetIds } = useMemo( - () => buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId: moveOwnerAccountId }), - [mailboxes, selectedMailbox, moveOwnerAccountId], + () => buildMoveTargets(mailboxes, { + currentMailboxId: selectedMailbox, + ownerAccountId: moveOwnerAccountId, + emails: email ? [email] : [], + }), + [mailboxes, selectedMailbox, moveOwnerAccountId, email], ); // Close dropdown menus on click outside diff --git a/components/email/move-to-dialog.tsx b/components/email/move-to-dialog.tsx index aab1ad4b7..591ef49c8 100644 --- a/components/email/move-to-dialog.tsx +++ b/components/email/move-to-dialog.tsx @@ -26,6 +26,7 @@ export function MoveToDialog({ email, mailboxes, selectedMailbox, onClose, onMov () => buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId: resolveMoveOwnerAccountId(email, mailboxes, selectedMailbox), + emails: email ? [email] : [], }), [email, mailboxes, selectedMailbox], ); diff --git a/components/email/mv-list-toolbar.tsx b/components/email/mv-list-toolbar.tsx index 7bf27916c..1b1cebb10 100644 --- a/components/email/mv-list-toolbar.tsx +++ b/components/email/mv-list-toolbar.tsx @@ -278,6 +278,7 @@ export function MvListToolbar({ const { tree: moveTree, targetIds: moveTargetIds } = buildMoveTargets(mailboxes, { currentMailboxId: selectedMailbox, ownerAccountId: resolveMoveOwnerAccountId(selectedEmails[0], mailboxes, selectedMailbox), + emails: selectedEmails, }); const renderMoveNodes = (nodes: MailboxNode[], depth = 0): React.ReactNode => diff --git a/lib/__tests__/move-targets.test.ts b/lib/__tests__/move-targets.test.ts index 58e66d63a..003a903f7 100644 --- a/lib/__tests__/move-targets.test.ts +++ b/lib/__tests__/move-targets.test.ts @@ -53,6 +53,25 @@ describe('buildMoveTargets', () => { const { tree } = buildMoveTargets([...mailboxes, other], { currentMailboxId: 'S:a', ownerAccountId: 'S' }); expect(tree.map((n) => n.id)).toEqual(['shared-account-S', `${OWN_ACCOUNT_MOVE_NODE_PREFIX}A`, 'shared-account-T']); }); + + it('offers the open folder for a search hit from another folder (#1452)', () => { + const hit = { mailboxIds: { c: true } } as unknown as Email; + const { tree, targetIds } = buildMoveTargets(mailboxes, { currentMailboxId: 'a', ownerAccountId: 'A', emails: [hit] }); + expect(targetIds.has('a')).toBe(true); + expect(tree.map((n) => n.id)).toContain('a'); + }); + + it('leaves the open folder out when every message is already in it', () => { + const inInbox = { mailboxIds: { a: true } } as unknown as Email; + const inBoth = { mailboxIds: { a: true, c: true } } as unknown as Email; + const elsewhere = { mailboxIds: { c: true } } as unknown as Email; + const opts = { currentMailboxId: 'a', ownerAccountId: 'A' }; + + expect(buildMoveTargets(mailboxes, { ...opts, emails: [inInbox, inBoth] }).targetIds.has('a')).toBe(false); + expect(buildMoveTargets(mailboxes, { ...opts, emails: [inInbox, elsewhere] }).targetIds.has('a')).toBe(true); + // The shared Inbox is a different folder for a message of the own Inbox. + expect(buildMoveTargets(mailboxes, { currentMailboxId: 'S:a', emails: [inInbox] }).targetIds.has('S:a')).toBe(true); + }); }); describe('resolveMoveOwnerAccountId', () => { diff --git a/lib/move-targets.ts b/lib/move-targets.ts index 28a1f7920..41a70c662 100644 --- a/lib/move-targets.ts +++ b/lib/move-targets.ts @@ -4,6 +4,35 @@ import { buildMailboxTree, type MailboxNode } from "./utils"; /** Header node that wraps the user's own folders when another account leads. */ export const OWN_ACCOUNT_MOVE_NODE_PREFIX = "own-account-"; +// Whether an email belongs to a given mailbox. +// Own-account emails carry bare ids (equal to both `id` and `originalId`). +// As of #281 V3, EVERY client fetch path (getEmails/getEmail/getThreadEmails/ +// searchEmails/advancedSearchEmails) namespaces shared/delegated emails' +// `mailboxIds` to the store id (`${ownerId}:${origId}`), so the `ids[mailbox.id]` +// fast path below matches own and shared mailboxes alike - one id space. +// The `originalId` branches remain as a defensive fallback for any email that +// still carries a bare owner id (scoped to the owning account via +// `sourceAccountId === mailbox.accountId` so a bare owner id can't collide with +// another account's folder). (#281) +export function emailInMailbox( + email: { mailboxIds?: Record; sourceAccountId?: string }, + mailbox: Mailbox, +): boolean { + const ids = email.mailboxIds; + if (!ids) return false; + if (ids[mailbox.id]) return true; + if (!mailbox.isShared && mailbox.originalId) return !!ids[mailbox.originalId]; + if ( + mailbox.isShared && + mailbox.originalId && + email.sourceAccountId && + email.sourceAccountId === mailbox.accountId + ) { + return !!ids[mailbox.originalId]; + } + return false; +} + /** * JMAP accountId of the account a message lives in, for scoping its Move menu. * @@ -34,16 +63,30 @@ export function resolveMoveOwnerAccountId( * shared one (#1149). The shared account's folders now come first and the * user's own folders follow under their account name, so a deliberate * cross-account move is still one pick away but no longer the obvious one. + * + * The open folder is left out only when the messages being moved are all in + * it. A search across folders lists messages from other folders while the + * open one stays selected, and those can be moved into it (#1452). */ export function buildMoveTargets( mailboxes: Mailbox[], - opts: { currentMailboxId?: string | null; ownerAccountId?: string }, + opts: { + currentMailboxId?: string | null; + ownerAccountId?: string; + /** The messages being moved; without them the open folder is left out. */ + emails?: Array>; + }, ): { tree: MailboxNode[]; targetIds: Set } { + const current = mailboxes.find((m) => m.id === opts.currentMailboxId); + const emails = opts.emails ?? []; + const hiddenId = current && (emails.length === 0 || emails.every((e) => emailInMailbox(e, current))) + ? current.id + : undefined; const targetIds = new Set( mailboxes .filter( (m) => - m.id !== opts.currentMailboxId && + m.id !== hiddenId && m.role !== "drafts" && !m.id.startsWith("shared-") && m.myRights?.mayAddItems diff --git a/stores/__tests__/email-store-plugin-organize-hooks.test.ts b/stores/__tests__/email-store-plugin-organize-hooks.test.ts index 46430a946..f4ce86f91 100644 --- a/stores/__tests__/email-store-plugin-organize-hooks.test.ts +++ b/stores/__tests__/email-store-plugin-organize-hooks.test.ts @@ -4,6 +4,7 @@ import { useAuthStore } from '../auth-store'; import { useSettingsStore } from '@/stores/settings-store'; import { clearAllHooks, emailHooks } from '@/lib/plugin-hooks'; import { emailKeyFor } from '@/lib/thread-utils'; +import { SEARCH_SCOPE_ALL_FOLDERS } from '@/lib/search-scope-folders'; import type { Email, Mailbox } from '@/lib/jmap/types'; import type { IJMAPClient } from '@/lib/jmap/client-interface'; @@ -82,6 +83,8 @@ function openFolder(folderId: string, emails: Email[]) { isUnifiedView: false, unifiedRole: null, crossView: null, + searchQuery: '', + searchMailboxId: folderId, emails, selectedEmail: null, selectedEmailKeys: new Set(), @@ -237,6 +240,20 @@ describe('move and archive hooks', () => { expect(archived).not.toHaveBeenCalled(); }); + it('reports the folder a search hit is in, not the open one (#1452)', async () => { + openFolder('archive', [makeEmail('e1', { mailboxIds: { custom: true } })]); + useEmailStore.setState({ searchQuery: 'invoice', searchMailboxId: SEARCH_SCOPE_ALL_FOLDERS }); + const after = vi.fn(); + const archived = vi.fn(); + emailHooks.onAfterEmailMove.register('t', after); + emailHooks.onEmailArchive.register('t', archived); + + expect(await useEmailStore.getState().moveToMailbox(client, 'e1', 'archive')).toBe(true); + + expect(after).toHaveBeenCalledWith({ emailIds: ['e1'], fromMailboxId: 'custom', toMailboxId: 'archive' }); + expect(archived).toHaveBeenCalledWith({ emailIds: ['e1'] }); + }); + it('batch archive asks and reports once for the selection', async () => { const a = makeEmail('a'); const b = makeEmail('b'); diff --git a/stores/email-store.ts b/stores/email-store.ts index eb30e431a..80efff194 100644 --- a/stores/email-store.ts +++ b/stores/email-store.ts @@ -18,6 +18,7 @@ import { emailHooks } from "@/lib/plugin-hooks"; import { scheduleAutoMarkAsRead } from "@/lib/mark-as-read-delay"; import { allowEmailMove, createMailboxWithHooks, emitEmailMove, idsFromSelectionKeys, keywordChangeLists, mailboxToView, quotaToView } from "@/lib/plugin-mail-hooks"; import { emailSummary, emailToReadView } from "@/lib/plugin-projection"; +import { emailInMailbox } from "@/lib/move-targets"; import { resolveThreadRoute } from "@/lib/thread-routing"; import { emailKeyFor, threadKeyFor, threadIdFromKey, KEYWORD_PREFIX, KEYWORD_PREFIX_LEGACY } from "@/lib/thread-utils"; import { provideSearchResults, type ProvidedSearchResult } from "@/lib/plugin-search-results"; @@ -2674,26 +2675,14 @@ async function refreshMailboxesForViewingAccount(fallbackClient: IJMAPClient): P } } -// Whether an email belongs to a given mailbox, for local counter math. -// Own-account emails carry bare ids (equal to both `id` and `originalId`). -// As of #281 V3, EVERY client fetch path (getEmails/getEmail/getThreadEmails/ -// searchEmails/advancedSearchEmails) namespaces shared/delegated emails' -// `mailboxIds` to the store id (`${ownerId}:${origId}`), so the `ids[mailbox.id]` -// fast path below matches own and shared mailboxes alike - one id space. -// The `originalId` branches remain as a defensive fallback for any email that -// still carries a bare owner id (scoped to the owning account via -// `sourceAccountId === mailbox.accountId` so a bare owner id can't collide with -// another account's folder). (#281) /** * The folder messages are moved out of, for the move hooks: the open folder, * or in an aggregate view (unified, tag, search across folders) the one * folder they all share, else null. */ function moveSourceMailboxId(emails: Array>, mailboxes: Mailbox[]): string | null { - const { selectedMailbox, selectedKeyword, isUnifiedView } = useEmailStore.getState(); - if (!selectedKeyword && !isUnifiedView && selectedMailbox && mailboxes.some(mb => mb.id === selectedMailbox)) { - return selectedMailbox; - } + const open = removalFolder(useEmailStore.getState()); + if (open && mailboxes.some(mb => mb.id === open.id)) return open.id; const sources = new Set(); for (const email of emails) { const mailbox = mailboxes.find(mb => emailInMailbox(email, mb)); @@ -2703,25 +2692,6 @@ function moveSourceMailboxId(emails: Array; sourceAccountId?: string }, - mailbox: Mailbox, -): boolean { - const ids = email.mailboxIds; - if (!ids) return false; - if (ids[mailbox.id]) return true; - if (!mailbox.isShared && mailbox.originalId) return !!ids[mailbox.originalId]; - if ( - mailbox.isShared && - mailbox.originalId && - email.sourceAccountId && - email.sourceAccountId === mailbox.accountId - ) { - return !!ids[mailbox.originalId]; - } - return false; -} - /** * The folder "Remove from this folder" takes messages out of (#615): the * folder the list shows, when it shows exactly one. A search scoped to one