From 2525c27a7d3dbf01ad5bc227d37376657ee23e58 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 4 Nov 2026 05:19:08 +0100 Subject: [PATCH] fix: group the message menus and keep app-level items out of the reader's more menu The reader's more menu no longer offers Star while the star sits by the subject, so the tests that expected Star in the menu and the phone panel now expect it gone. Import mail moved to the command palette (it stays in the folder menu and Settings); Keyboard shortcuts stay behind ?, the rail and Help. --- .../__tests__/message-action-list.test.tsx | 47 +++++++++++++++++-- components/email/email-context-menu.tsx | 6 ++- components/email/email-viewer.tsx | 26 ++++------ components/email/message-actions.ts | 24 +++++++--- .../__tests__/mail-palette-commands.test.tsx | 13 +++++ components/mail/mail-app.tsx | 8 +++- 6 files changed, 92 insertions(+), 32 deletions(-) diff --git a/components/email/__tests__/message-action-list.test.tsx b/components/email/__tests__/message-action-list.test.tsx index 9233cd61f..3abd6d75a 100644 --- a/components/email/__tests__/message-action-list.test.tsx +++ b/components/email/__tests__/message-action-list.test.tsx @@ -1,4 +1,4 @@ -import { render, screen, fireEvent, act, within } from '@testing-library/react'; +import { render, renderHook, screen, fireEvent, act, within } from '@testing-library/react'; import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Email, Mailbox } from '@/lib/jmap/types'; @@ -28,6 +28,7 @@ const { EmailViewer } = await import('../email-viewer'); const { EmailContextMenu } = await import('../email-context-menu'); const { usePrintRequestStore, requestPrint } = await import('@/hooks/use-message-file-actions'); const { emailKeyFor } = await import('@/lib/thread-utils'); +const { useMessageActionList } = await import('../message-actions'); const email = { id: 'e1', @@ -112,17 +113,53 @@ describe("the reader's ⋮ on a desktop", () => { renderViewer({ onTogglePinned }); fireEvent.click(screen.getByRole('button', { name: 'more_actions' })); const menu = screen.getByRole('menu', { name: 'more_actions' }); - for (const id of ['ctx-star', 'ctx-move-to', 'ctx-tag', 'ctx-export', 'ctx-print', 'ctx-view-source']) { + for (const id of ['ctx-move-to', 'ctx-tag', 'ctx-export', 'ctx-print', 'ctx-view-source']) { expect(within(menu).getByTestId(id)).toBeTruthy(); } - // Delete is on the toolbar already. + // Delete is on the toolbar already, and the star by the subject. expect(within(menu).queryByTestId('ctx-delete')).toBeNull(); + expect(within(menu).queryByTestId('ctx-star')).toBeNull(); fireEvent.click(within(menu).getByRole('menuitem', { name: 'pin' })); expect(onTogglePinned).toHaveBeenCalledTimes(1); expect(screen.queryByRole('menu', { name: 'more_actions' })).toBeNull(); }); }); +describe("the reader's ⋮ in groups", () => { + it("has the message's actions only, in groups", () => { + renderViewer({ onTogglePinned: vi.fn(), onShowShortcuts: vi.fn() } as never); + fireEvent.click(screen.getByRole('button', { name: 'more_actions' })); + const menu = screen.getByRole('menu', { name: 'more_actions' }); + const rows = Array.from(menu.querySelectorAll('[role="menuitem"], [role="separator"]')) + .map((row) => (row.getAttribute('role') === 'separator' ? '|' : row.querySelector('[data-menu-label]')?.textContent ?? '')); + // No app-level entries: Import is in the folder's menu, Settings and the + // palette, the shortcuts behind ?. Answer; file away; mark; spam; link; + // how it shows; save. + expect(rows).toEqual([ + 'reply', 'reply_all', 'forward', 'forward_as_attachment', '|', + 'archive', '|', + 'move_to', '|', + 'mark_unread', 'pin', 'tag', '|', + 'mark_as_spam', '|', + 'copy_message', 'copy_conversation', '|', + 'reader_view', '|', + 'export_email', 'print', 'view_source', + ]); + }); + + it('puts how the reader shows the message in a group before Export, Print and View source', () => { + const { result } = renderHook(() => useMessageActionList({ + email, + onExport: () => {}, + onPrint: () => {}, + views: [{ kind: 'item', key: 'extra:reader_view', icon: () => null, label: 'reader_view', run: () => {} }], + extras: [{ kind: 'item', key: 'extra:other', icon: () => null, label: 'other', run: () => {} }], + })); + const labels = result.current.map((entry) => (entry.kind === 'separator' ? '|' : entry.label)); + expect(labels.slice(-7)).toEqual(['|', 'reader_view', '|', 'export_email', 'print', '|', 'other']); + }); +}); + describe("the reader toolbar's Move and Tag menus", () => { it('are the ContextMenu: in a portal, with its rows and keys', () => { renderViewer(); @@ -149,7 +186,9 @@ describe("the phone's More panel", () => { const panel = screen.getByRole('menu', { name: 'more_actions' }); const pin = within(panel).getByTestId('panel-pin'); expect(pin.className).toContain('min-h-[44px]'); - for (const id of ['panel-star', 'panel-export', 'panel-print', 'panel-view_source', 'panel-copy_message_link']) { + // The star is by the subject, on the phone too. + expect(within(panel).queryByTestId('panel-star')).toBeNull(); + for (const id of ['panel-export', 'panel-print', 'panel-view_source', 'panel-copy_message_link']) { expect(within(panel).getByTestId(id)).toBeTruthy(); } fireEvent.click(pin); diff --git a/components/email/email-context-menu.tsx b/components/email/email-context-menu.tsx index a27531ead..b7205a3f6 100644 --- a/components/email/email-context-menu.tsx +++ b/components/email/email-context-menu.tsx @@ -131,7 +131,9 @@ interface EmailContextMenuProps { onViewSource?: () => void; /** Entries the host already shows elsewhere (the reader's toolbar). */ omit?: ReadonlySet; - /** The host's own entries after the message's (the reader's view toggles). */ + /** How the host shows the message (the reader's view toggles), before Export, Print and View source. */ + views?: MessageActionEntry[]; + /** The host's own entries after the message's. */ extras?: MessageActionEntry[]; /** The row's plugin entries (message-row-actions). Default true. */ rowPlugins?: boolean; @@ -192,6 +194,7 @@ export function EmailContextMenu({ onPrint, onViewSource, omit, + views, extras, rowPlugins = true, extraEntries, @@ -319,6 +322,7 @@ export function EmailContextMenu({ onPrint, onViewSource, omit, + views, extras, }); diff --git a/components/email/email-viewer.tsx b/components/email/email-viewer.tsx index 0b1b5fac7..b8b4e3ede 100644 --- a/components/email/email-viewer.tsx +++ b/components/email/email-viewer.tsx @@ -3,7 +3,6 @@ import { Fragment, useState, useEffect, useLayoutEffect, useMemo, useRef, useCallback, useId } from "react"; import { Email, ContactCard, Mailbox } from "@/lib/jmap/types"; import { attachmentDownloadFilename, attachmentsBundleFilename, DEFAULT_ATTACHMENT_TEMPLATE } from "@/lib/download-filename"; -import { openMailImport } from "@/stores/mail-transfer-store"; import { TRANSPARENT_BLOCKED_PIXEL, applyNewTabToAnchor, emailIframeCsp, escapeHtml, isExternalResourceUrl, isOpenableLinkHref, plainTextToSafeHtml, sanitizeEmailBodyForIframe, sanitizeEmailHtml, sanitizePlainTextRenderedHtml } from "@/lib/email-sanitization"; import { getRenderableHtmlBody } from "@/lib/email-body-selection"; import { mailLanguage } from "@/lib/mail-language"; @@ -106,7 +105,6 @@ import { Send, FolderInput, Sun, - Upload, Moon, PlayCircle, PenSquare, @@ -278,7 +276,6 @@ interface EmailViewerProps { onReplyAllConversationEmail?: (email: Email) => void; /** Mountain View: a reply being written in place, shown under the conversation instead of the reply buttons. */ mvInlineComposer?: React.ReactNode; - onShowShortcuts?: () => void; onEditDraft?: () => void; onCancelScheduledForEdit?: () => void; /** Stop a scheduled message from going out; it goes back to Drafts. */ @@ -1157,6 +1154,7 @@ function ReaderMoreMenu({ menu, options, label, mailboxes, selectedMailbox, onMo onPrint={options.onPrint} onViewSource={options.onViewSource} omit={options.omit} + views={options.views} extras={options.extras} rowPlugins={false} extraEntries={pluginEntries} @@ -1341,7 +1339,6 @@ export function EmailViewer({ onReplyConversationEmail, onReplyAllConversationEmail, mvInlineComposer, - onShowShortcuts, onEditDraft, onCancelScheduledForEdit, onCancelScheduledSend, @@ -1364,7 +1361,6 @@ export function EmailViewer({ const tWindows = useTranslations('pwaWindows'); const tComposer = useTranslations('email_composer'); const tNotifications = useTranslations('notifications'); - const tImport = useTranslations('mail_import'); const tCommon = useTranslations('common'); const tFiles = useTranslations('files'); const tDemoWelcome = useTranslations('demo_welcome'); @@ -3768,11 +3764,6 @@ export function EmailViewer({ await exportEmail(email, { onArchive, onDelete, archiveEnabled: archiveActionEnabled }, blobSource ?? { client, accountId: undefined }); }; - // Import mail into the open folder: the import dialog (lib/mail-import) does the rest. - const handleImportEmail = () => { - openMailImport({ mailboxId: useEmailStore.getState().selectedMailbox || undefined }); - }; - // What a printout of the message shows, for the Print action's window and // for the page's print copy (Ctrl+P). Text fields are unescaped. const messagePrintFields = (): MessagePrintFields | null => { @@ -4228,6 +4219,8 @@ ${printWatermark ? printWatermark.html : ''} // the bottom bar has the replies. const toolbarShownActions = (): ReadonlySet => { const shown = new Set(['delete', 'edit_draft']); + // The star sits by the subject, or on the sender's line in Mountain View. + if (toolbarPosition === 'top' || mvViewer) shown.add('star'); toolbarRef.current?.querySelectorAll('[data-overflow-item]').forEach((item) => { const key = TOOLBAR_PRIORITY_ACTIONS[item.dataset.overflowPriority ?? '']; if (key && item.getClientRects().length > 0) shown.add(key); @@ -4248,9 +4241,10 @@ ${printWatermark ? printWatermark.html : ''} return hidden; }; const toggleViewMode = () => setEmailViewDarkOverride(prev => prev === null ? !(resolvedTheme === 'dark') : !prev); - // The reader's own entries after the message's: the view toggle, Import - // (into the open folder) and, with a keyboard, the shortcuts. - const readerExtras: MessageActionEntry[] = [ + // How the reader shows the message, a group of the ⋮ menu of its own. Import + // and the keyboard shortcuts are no message's: Import is in the folder's menu, + // Settings and the command palette, the shortcuts behind ?, the rail and Help. + const readerViews: MessageActionEntry[] = [ ...(readAloud.supported && !isBodyLoading ? [{ kind: 'item' as const, key: 'extra:read_aloud' as const, icon: Volume2, label: t('read_aloud'), run: readAloud.start }] : []), @@ -4263,10 +4257,6 @@ ${printWatermark ? printWatermark.html : ''} ...(effectiveEmailContent.isHtml ? [{ kind: 'item' as const, key: 'extra:view_mode' as const, icon: isDark ? Sun : Moon, label: isDark ? t('view_in_light_mode') : t('view_in_dark_mode'), run: toggleViewMode }] : []), - { kind: 'item', key: 'extra:import', icon: Upload, label: tImport('menu_import'), run: handleImportEmail }, - ...(onShowShortcuts && !isMobile - ? [{ kind: 'item' as const, key: 'extra:shortcuts' as const, icon: Keyboard, label: t('keyboard_shortcuts'), run: onShowShortcuts }] - : []), ]; const readerActionOptions: MessageActionOptions = { email, @@ -4291,7 +4281,7 @@ ${printWatermark ? printWatermark.html : ''} onPrint: devices.noPrint ? undefined : () => handlePrint(), onViewSource: devices.noExport ? undefined : () => setShowSourceModal(true), omit: readerMenuOmit, - extras: readerExtras.filter((entry) => entry.kind !== 'item' || !readerMenuOmit.has(entry.key)), + views: readerViews, }; // Shared toolbar items used by both 'top' and 'below-subject' positions diff --git a/components/email/message-actions.ts b/components/email/message-actions.ts index 8fafc6ba2..b9078819a 100644 --- a/components/email/message-actions.ts +++ b/components/email/message-actions.ts @@ -125,6 +125,8 @@ export interface MessageActionOptions { onViewSource?: () => void; /** Entries the host already shows elsewhere (the reader's toolbar). */ omit?: ReadonlySet; + /** How the host shows the message (the reader's view toggles): a group before Export, Print and View source. */ + views?: MessageActionEntry[]; /** The host's own entries, after the message's own and a separator. */ extras?: MessageActionEntry[]; } @@ -230,7 +232,6 @@ export function useMessageActionList(o: MessageActionOptions): MessageActionEntr void copyLink(buildMailPath({ mailboxId: null, emailId: null, threadId: email.threadId!, ...emailLinkSource(email) })); })] : []), - SEPARATOR, ]; const archive = archiveEnabled ? [item("archive", Archive, t("archive"), o.onArchive)] : []; const del = item("delete", Trash2, t("delete"), o.onDelete, { destructive: !mountainView, testId: "ctx-delete" }); @@ -267,25 +268,34 @@ export function useMessageActionList(o: MessageActionOptions): MessageActionEntr const find = o.onFindFromSender && sender?.email ? [SEPARATOR, item("find_from_sender", Search, t("find_emails_from", { name: sender.name || sender.email }), o.onFindFromSender, { testId: "ctx-find-from" })] : []; - // Download/Export, Print, View source (R6: after Copy, before the end). + const openWindow = o.onOpenInWindow + ? [item("open_window", ExternalLink, tWindows("open_in_window"), o.onOpenInWindow, { testId: "ctx-open-window" })] + : []; + // How the host shows it, then Download/Export, Print, View source, at the end. + const views = o.views ?? []; const file = [ - ...(o.onOpenInWindow ? [item("open_window", ExternalLink, tWindows("open_in_window"), o.onOpenInWindow, { testId: "ctx-open-window" })] : []), ...(o.onExport ? [item("export", Download, tViewer("export_email"), o.onExport, { disabled: !email.blobId, testId: "ctx-export" })] : []), ...(o.onPrint ? [item("print", Printer, tViewer("print"), o.onPrint, { testId: "ctx-print" })] : []), ...(o.onViewSource ? [item("view_source", Code, tViewer("view_source"), o.onViewSource, { disabled: !email.blobId, testId: "ctx-view-source" })] : []), ]; + // In groups, as mail clients have them: answer; file away; mark; spam; + // open and link; how it shows; save, print, source. list = mountainView ? [ ...draft, ...replies, ...archive, del, read, SEPARATOR, ...move, tag, ...rules, ...copy, SEPARATOR, star, ...pin, ...spam, ...reportPhishing, ...find, SEPARATOR, - ...links, ...file, + ...links, SEPARATOR, ...views, SEPARATOR, ...openWindow, ...file, ] : [ - ...draft, ...replies, ...links, ...archive, del, SEPARATOR, - ...move, ...copy, ...rules, star, ...pin, tag, ...spam, ...reportPhishing, SEPARATOR, - read, SEPARATOR, ...file, + ...draft, ...replies, ...archive, del, SEPARATOR, + ...move, ...copy, ...rules, SEPARATOR, + read, star, ...pin, tag, SEPARATOR, + ...spam, ...reportPhishing, SEPARATOR, + ...openWindow, ...links, SEPARATOR, + ...views, SEPARATOR, + ...file, ]; } diff --git a/components/mail/__tests__/mail-palette-commands.test.tsx b/components/mail/__tests__/mail-palette-commands.test.tsx index b630376c6..d19546878 100644 --- a/components/mail/__tests__/mail-palette-commands.test.tsx +++ b/components/mail/__tests__/mail-palette-commands.test.tsx @@ -8,6 +8,7 @@ import { useSettingsStore } from '@/stores/settings-store'; import { usePluginStore } from '@/stores/plugin-store'; import { useThemeStore } from '@/stores/theme-store'; import { useToastStore } from '@/stores/toast-store'; +import { useMailTransferStore } from '@/stores/mail-transfer-store'; import { DEFAULT_SEARCH_FILTERS } from '@/lib/jmap/search-utils'; import { clearAllHooks } from '@/lib/plugin-hooks'; import type { CommandGroup } from '@/lib/command-registry'; @@ -108,6 +109,18 @@ describe('Mail in the command palette', () => { expect(ids).not.toContain('mark-read'); }); + it("offers Import mail into the open folder, which left the reader's ⋮ menu", async () => { + let groups: CommandGroup[] = []; + render(<> { groups = g; }} />); + await screen.findByText('viewer', {}, { timeout: 5000 }); + await waitFor(() => expect(groups.find((g) => g.label === 'sidebar.mail')).toBeDefined()); + const importMail = groups.find((g) => g.label === 'sidebar.mail')!.commands.find((c) => c.id === 'import'); + expect(importMail?.label).toBe('mail_import.menu_import'); + act(() => importMail!.run()); + expect(useMailTransferStore.getState().request).toMatchObject({ kind: 'import', mailboxId: 'inbox' }); + act(() => useMailTransferStore.setState({ request: null, visible: false })); + }); + it("says whose a shared folder is and where a subfolder is, so two Inboxes don't look the same", async () => { const shared = { ...folder('support:inbox', 'inbox'), isShared: true, accountName: 'Support', accountId: 'support', originalId: 'inbox' } as Mailbox; const archive = folder('archive', 'archive', 'Archive'); diff --git a/components/mail/mail-app.tsx b/components/mail/mail-app.tsx index 11d4246c0..555d2b7ca 100644 --- a/components/mail/mail-app.tsx +++ b/components/mail/mail-app.tsx @@ -136,7 +136,7 @@ import { computeReplyThreadingHeaders } from "@/lib/email-threading"; import { openMailImport } from "@/stores/mail-transfer-store"; import { deliveryAddresses, resolveComposeAccountEmail } from "@/lib/reply-identity"; import { useProMultiAccountIdentities } from "@/hooks/use-pro-multi-account-identities"; -import { Filter, X, Mail, MailOpen, PenSquare, CheckSquare, Square, AlertTriangle, ArrowLeft, RefreshCw, Reply, ReplyAll, Forward, Archive, Trash2, FolderInput, ShieldAlert, ShieldCheck, Folder } from "@/components/icons"; +import { Filter, X, Mail, MailOpen, PenSquare, CheckSquare, Square, AlertTriangle, ArrowLeft, RefreshCw, Reply, ReplyAll, Forward, Archive, Trash2, FolderInput, ShieldAlert, ShieldCheck, Folder, Upload } from "@/components/icons"; import { useRegisterCommands } from "@/lib/command-registry"; import { MoveToDialog } from "@/components/email/move-to-dialog"; import { ResizeHandle } from "@/components/layout/resize-handle"; @@ -4819,6 +4819,11 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { when: () => !!paletteEmail && conversationSpamApplicable, run: () => void (conversationInJunk ? handleUndoSpam(paletteEmail) : handleMarkAsSpam(paletteEmail)), }, + // Into the open folder, as the folder's own menu does it. + { + id: 'import', label: t('mail_import.menu_import'), icon: Upload, + run: () => openMailImport({ mailboxId: selectedMailbox || undefined }), + }, ...mailboxes .filter((mb) => !mb.id.startsWith('shared-') && mb.id !== selectedMailbox) .map((mb) => { @@ -6113,7 +6118,6 @@ export function MailApp({ linkSegments: routeSegments }: MailAppProps = {}) { onToggleFullscreen={!isEmbedded && !isMobile ? () => setFullscreenReading((v) => !v) : undefined} onOpenInWindow={!isEmbedded && !isMobile && openEmailInWindow && selectedEmail ? () => openEmailInWindow(selectedEmail) : undefined} isFullscreen={fullscreenReading} - onShowShortcuts={() => setShowShortcutsModal(true)} onEditDraft={handleEditDraft} onCancelScheduledForEdit={async () => { if (!client || !selectedEmail) return;