fix: let search results move into the open folder #1452

This commit is contained in:
Linus Rath
2026-10-31 01:16:34 +01:00
parent d2f9f1a1a4
commit dafdda5347
10 changed files with 176 additions and 39 deletions
@@ -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> = {}): 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<React.ComponentProps<typeof EmailContextMenu>>) {
const result = render(
<EmailContextMenu
email={inWork}
position={{ x: 0, y: 0 }}
isOpen
onClose={() => {}}
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');
});
});
@@ -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;
+1
View File
@@ -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
+6 -2
View File
@@ -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
+1
View File
@@ -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],
);
+1
View File
@@ -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 =>
+19
View File
@@ -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', () => {
+45 -2
View File
@@ -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<string, boolean>; 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<Pick<Email, "mailboxIds" | "sourceAccountId">>;
},
): { tree: MailboxNode[]; targetIds: Set<string> } {
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
@@ -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');
+3 -33
View File
@@ -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<Pick<Email, 'mailboxIds' | 'sourceAccountId'>>, 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<string>();
for (const email of emails) {
const mailbox = mailboxes.find(mb => emailInMailbox(email, mb));
@@ -2703,25 +2692,6 @@ function moveSourceMailboxId(emails: Array<Pick<Email, 'mailboxIds' | 'sourceAcc
return sources.size === 1 ? [...sources][0] : null;
}
function emailInMailbox(
email: { mailboxIds?: Record<string, boolean>; 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