diff --git a/FEATURES.md b/FEATURES.md index 7420521..d94de1d 100644 --- a/FEATURES.md +++ b/FEATURES.md @@ -152,6 +152,26 @@ more attempt. A drag that is merely more sideways than not stays a scroll. message count; it can be switched off to list messages individually. - **Multi-select** with `x`, shift-click for ranges, `Ctrl/Cmd+A` for all, and a long press on a touchscreen. +- **Select the whole folder**, not just the rows that happen to be loaded. The + header checkbox takes the loaded page, which on a folder of ten thousand is + fifty of them; a line then offers the other 9,950 by name, and taking it is a + separate press. A checkbox that silently meant ten thousand when the screen + shows fifty would be the worst of both, so each option says what it actually + covers. + + The wider selection is a *query*, not a list of ids: what it reaches is + resolved from the server when an action runs, walked a page at a time, + because a folder holds far more than one call returns. It is resolved + **uncollapsed** — "everything in this folder" means every message rather than + one per thread. And it is consumed by the action that used it, so the next + action does not silently reach the whole folder again. + + **Undo is withheld once the selection reaches messages that were never + loaded.** Undo restores the folders each message was in, which can only be + known for messages the browser holds; built from the others it would write an + empty set of folders and leave the message in none at all. Withholding the + offer is better than restoring something wrong, and the toast simply does not + carry it. - **Drag and drop** onto any folder in the tree, moving the selection or the row under the cursor. - **Context menu** on any row: reply, forward, archive, delete, spam, read/unread, diff --git a/web/src/store/__tests__/select-all-in-folder.test.ts b/web/src/store/__tests__/select-all-in-folder.test.ts new file mode 100644 index 0000000..56d66bf --- /dev/null +++ b/web/src/store/__tests__/select-all-in-folder.test.ts @@ -0,0 +1,191 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { CAP, client } from "@/jmap/client"; +import { useMail } from "@/store/mail"; +import { useToasts } from "@/ui/toast"; +import type { JmapSession } from "@/jmap/types"; + +/** + * Selecting a whole folder rather than the rows that happen to be loaded. + * + * Two things are worth testing beyond the flag itself: that the ids are walked + * a page at a time (a folder can hold far more than one call returns), and + * that Undo is withheld once the selection reaches messages that were never + * loaded -- an Undo built from those would write an empty mailboxIds and put + * the message in no folder at all. + */ + +const PAGE = 3; // small, so paging is exercised without fixtures the size of a mailbox +const INBOX = "mbInbox"; +const ARCHIVE = "mbArchive"; + +function server(totalIds: number) { + const all = Array.from({ length: totalIds }, (_, i) => `e${i}`); + const queries: Array<{ position: number; limit: number; collapseThreads: unknown }> = []; + const updates: Array> = []; + + const fetchMock = vi.fn(async (_url: string, init: RequestInit) => { + const body = JSON.parse(init.body as string) as { methodCalls: [string, Record, string][] }; + const methodResponses = body.methodCalls.map(([name, args, id]) => { + if (name === "Email/query") { + const position = (args.position as number) ?? 0; + const limit = (args.limit as number) ?? PAGE; + queries.push({ position, limit, collapseThreads: args.collapseThreads }); + return [name, { accountId: "a1", queryState: "q", canCalculateChanges: false, position, ids: all.slice(position, position + limit), total: all.length }, id]; + } + if (name === "Email/set" && args.update) { + updates.push(args.update as Record); + return [name, { accountId: "a1", oldState: "1", newState: "2", updated: {}, notUpdated: {} }, id]; + } + return [name, { accountId: "a1", state: "1", list: [], notFound: [], ids: [], total: 0, queryState: "q", position: 0, canCalculateChanges: false }, id]; + }); + return { ok: true, status: 200, json: async () => ({ methodResponses, sessionState: "1" }) } as Response; + }); + vi.stubGlobal("fetch", fetchMock); + return { all, queries, updates }; +} + +const toastActions = () => useToasts.getState().toasts.map((t) => t.action?.label ?? null); + +beforeEach(() => { + client.session = { + capabilities: { [CAP.core]: { maxObjectsInGet: PAGE, maxObjectsInSet: PAGE }, [CAP.mail]: {} }, + accounts: {}, + primaryAccounts: {}, + state: "s1", + } as unknown as JmapSession; + useMail.setState({ + accountId: "a1", + mailboxes: { + [INBOX]: { id: INBOX, role: "inbox", name: "Inbox", parentId: null }, + [ARCHIVE]: { id: ARCHIVE, role: "archive", name: "Archive", parentId: null }, + } as never, + emails: {}, + selected: {}, + selectedAll: false, + list: { + key: "k", + filter: { inMailbox: INBOX }, + sort: [], + collapseThreads: true, + mailboxId: INBOX, + ids: ["e0", "e1", "e2"], + total: 8, + queryState: "q", + loading: false, + loadingMore: false, + error: null, + exhausted: false, + } as never, + }); + useToasts.setState({ toasts: [] }); +}); + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +describe("selecting the loaded rows versus the whole folder", () => { + it("selectAll takes the loaded rows and nothing wider", () => { + useMail.getState().selectAll(); + expect(Object.keys(useMail.getState().selected)).toEqual(["e0", "e1", "e2"]); + // A checkbox that silently meant the whole folder would be the worst of both. + expect(useMail.getState().selectedAll).toBe(false); + }); + + it("selectAllMatching is the deliberate second step", () => { + useMail.getState().selectAll(); + useMail.getState().selectAllMatching(); + expect(useMail.getState().selectedAll).toBe(true); + }); + + it("clearing drops both", () => { + useMail.getState().selectAll(); + useMail.getState().selectAllMatching(); + useMail.getState().clearSelection(); + expect(useMail.getState().selected).toEqual({}); + expect(useMail.getState().selectedAll).toBe(false); + }); + + it("does nothing without a list to select from", () => { + useMail.setState({ list: null }); + useMail.getState().selectAllMatching(); + expect(useMail.getState().selectedAll).toBe(false); + }); +}); + +describe("queryAllIds", () => { + it("walks the folder a page at a time and returns every id", async () => { + const s = server(8); + const ids = await useMail.getState().queryAllIds(); + expect(ids).toEqual(s.all); + expect(s.queries.map((q) => q.position)).toEqual([0, 3, 6]); + }); + + it("stops on a short page rather than asking again to be told the same thing", async () => { + const s = server(6); + await useMail.getState().queryAllIds(); + // 6 ids over pages of 3 is two full pages, then one more that comes back + // empty -- the loop cannot know page two was the last without asking. + expect(s.queries.map((q) => q.position)).toEqual([0, 3, 6]); + }); + + it("asks uncollapsed, because the folder means every message and not every thread", async () => { + const s = server(3); + await useMail.getState().queryAllIds(); + expect(s.queries.every((q) => q.collapseThreads === false)).toBe(true); + // The list itself is collapsed; this deliberately is not. + expect(useMail.getState().list?.collapseThreads).toBe(true); + }); + + it("returns nothing when there is no list", async () => { + server(8); + useMail.setState({ list: null }); + expect(await useMail.getState().queryAllIds()).toEqual([]); + }); +}); + +describe("Undo, once the selection reaches messages that were never loaded", () => { + it("is offered when every message is loaded", async () => { + server(2); + useMail.setState({ + emails: { + e0: { id: "e0", mailboxIds: { [INBOX]: true }, keywords: {}, threadId: "t0" }, + e1: { id: "e1", mailboxIds: { [INBOX]: true }, keywords: {}, threadId: "t1" }, + } as never, + }); + await useMail.getState().move(["e0", "e1"], ARCHIVE); + expect(toastActions()).toContain("Undo"); + }); + + it("is withheld when any message is not loaded", async () => { + server(2); + useMail.setState({ emails: { e0: { id: "e0", mailboxIds: { [INBOX]: true }, keywords: {}, threadId: "t0" } } as never }); + // e1 was never loaded: its previous folders are unknown, and an Undo built + // from them would write an empty mailboxIds. + await useMail.getState().move(["e0", "e1"], ARCHIVE); + expect(toastActions()).not.toContain("Undo"); + expect(useToasts.getState().toasts).toHaveLength(1); + }); + + it("still performs the move itself", async () => { + const s = server(2); + await useMail.getState().move(["e0", "e1"], ARCHIVE); + const moved = s.updates.flatMap((u) => Object.entries(u)); + expect(moved.map(([id]) => id).sort()).toEqual(["e0", "e1"]); + for (const [, patch] of moved) { + expect((patch as { mailboxIds: Record }).mailboxIds).toEqual({ [ARCHIVE]: true }); + } + }); +}); + +describe("the wider selection does not outlive the action that used it", () => { + it("is dropped after a move, so the next action does not silently reach the folder again", async () => { + server(2); + useMail.getState().selectAll(); + useMail.getState().selectAllMatching(); + await useMail.getState().move(["e0"], ARCHIVE); + expect(useMail.getState().selectedAll).toBe(false); + expect(useMail.getState().selected).toEqual({}); + }); +}); diff --git a/web/src/store/mail.ts b/web/src/store/mail.ts index 74d35fa..1b25204 100644 --- a/web/src/store/mail.ts +++ b/web/src/store/mail.ts @@ -133,6 +133,12 @@ export interface MailState { selected: Record; /** Unread messages per label keyword, for the sidebar. */ labelCounts: Record; + /** + * The selection means "everything the current query matches", not the rows + * that happen to be loaded. Ticking the header box selects the loaded page; + * this is the deliberate second step past it. + */ + selectedAll: boolean; anchorId: Id | null; loadingThreads: Record; lastSeenInboxEmailIds: Id[] | null; @@ -190,6 +196,10 @@ export interface MailState { /** Refresh the per-label unread counts, in one request. */ loadLabelCounts(): Promise; selectAll(): void; + /** Extend the selection from the loaded rows to everything the query matches. */ + selectAllMatching(): void; + /** Every id the current query matches, walked a page at a time. */ + queryAllIds(): Promise; setAnchor(id: Id | null): void; applyChanges(types: Set): Promise; @@ -217,6 +227,7 @@ export const useMail = create((set, get) => ({ list: null, selected: {}, labelCounts: {}, + selectedAll: false, anchorId: null, loadingThreads: {}, lastSeenInboxEmailIds: null, @@ -242,6 +253,7 @@ export const useMail = create((set, get) => ({ vacation: null, list: null, selected: {}, + selectedAll: false, anchorId: null, lastSeenInboxEmailIds: null, }); @@ -298,6 +310,7 @@ export const useMail = create((set, get) => ({ set({ list: { ...q, key, ids: reuse ? cur.ids : [], total: reuse ? cur.total : 0, queryState: null, loading: true, loadingMore: false, error: null, exhausted: false }, selected: {}, + selectedAll: false, anchorId: null, }); try { @@ -477,8 +490,18 @@ export const useMail = create((set, get) => ({ const { emails, mailboxes } = get(); const prev: Record> = {}; const update: Record> = {}; + /* + * Undo restores the folders each message was in, which can only be offered + * for messages we actually hold. Selecting a whole folder reaches messages + * that were never loaded, and an Undo built from those would write an empty + * mailboxIds -- putting the message in no folder at all, which is worse + * than the move it was undoing. So the offer is withheld rather than + * quietly restoring something wrong. + */ + let undoable = true; for (const id of ids) { const e = emails[id]; + if (!e) undoable = false; prev[id] = e?.mailboxIds ?? {}; update[id] = { mailboxIds: { [toMailboxId]: true } }; } @@ -486,7 +509,7 @@ export const useMail = create((set, get) => ({ set((s) => { const next = { ...s.emails }; for (const id of ids) if (next[id]) next[id] = { ...next[id]!, mailboxIds: { [toMailboxId]: true } }; - return { emails: next, selected: {} }; + return { emails: next, selected: {}, selectedAll: false }; }); removeFromList(ids, set, get, toMailboxId); try { @@ -501,7 +524,7 @@ export const useMail = create((set, get) => ({ // is looking at in the sidebar rather than the server's own word for it. const name = mailboxDisplayName(mailboxes[toMailboxId]) || opts.label || t("folder"); toast.show(`${ids.length === 1 ? "Conversation" : `${ids.length} conversations`} moved to ${name}`, { - action: { + action: !undoable ? undefined : { label: "Undo", onClick: async () => { const undo: Record> = {}; @@ -569,7 +592,7 @@ export const useMail = create((set, get) => ({ set((s) => { const next = { ...s.emails }; for (const id of ids) delete next[id]; - return { emails: next, selected: {} }; + return { emails: next, selected: {}, selectedAll: false }; }); try { const { notDestroyed } = await destroyEmails(accountId, ids); @@ -606,7 +629,13 @@ export const useMail = create((set, get) => ({ // Where everything came from, captured before anything moves, so one Undo // can put back a selection that went to several folders. const prev: Record> = {}; - for (const id of ids) prev[id] = emails[id]?.mailboxIds ?? {}; + // See the note in move(): an Undo for a message we never loaded would + // write an empty mailboxIds, so it is not offered at all. + let undoable = true; + for (const id of ids) { + if (!emails[id]) undoable = false; + prev[id] = emails[id]?.mailboxIds ?? {}; + } const moved: string[] = []; try { @@ -632,7 +661,7 @@ export const useMail = create((set, get) => ({ ? t("Conversation moved to {folder}", { folder: where }) : t("{count} conversations moved to {folder}", { count: String(ids.length), folder: where }), { - action: { + action: !undoable ? undefined : { label: "Undo", onClick: async () => { const undo: Record> = {}; @@ -939,14 +968,60 @@ export const useMail = create((set, get) => ({ }, clearSelection() { - set({ selected: {} }); + set({ selected: {}, selectedAll: false }); }, selectAll() { const l = get().list; if (!l) return; const next: Record = {}; for (const id of l.ids) next[id] = true; - set({ selected: next }); + // Ticking the box is the loaded rows. Going wider is a separate, + // deliberate press, because "select all" meaning ten thousand messages + // when the screen shows fifty is not something to infer from a checkbox. + set({ selected: next, selectedAll: false }); + }, + + selectAllMatching() { + if (!get().list) return; + set({ selectedAll: true }); + }, + + async queryAllIds() { + const { accountId, list } = get(); + if (!accountId || !list) return []; + const page = client.maxObjectsInSet; + const out: Id[] = []; + let progress: number | null = null; + try { + for (let position = 0; ; position += page) { + const q = await client.call("Email/query", { + accountId, + filter: list.filter, + sort: list.sort, + /* + * Uncollapsed, unlike the list itself. "Everything in this folder" + * means every message; the list shows one row per thread only so it + * reads well. Expanding threads the way a click does is not possible + * here anyway -- that walks loaded Email objects, and the whole point + * is the ones that were never loaded. + */ + collapseThreads: false, + position, + limit: page, + }); + if (!q.ids.length) break; + out.push(...q.ids); + if (progress === null && q.ids.length === page) { + progress = toast.show(t("Working out what is selected…"), { duration: 0 }); + } + // A short page is the last page. Asking again would cost a round trip + // to be told the same thing. + if (q.ids.length < page) break; + } + } finally { + if (progress !== null) toast.dismiss(progress); + } + return out; }, setAnchor(id) { set({ anchorId: id }); diff --git a/web/src/styles/app.css b/web/src/styles/app.css index 92ad004..03abe44 100644 --- a/web/src/styles/app.css +++ b/web/src/styles/app.css @@ -1401,3 +1401,6 @@ button.dp-open:disabled { cursor: default; opacity: .5; } /* The banner naming a sender from outside the organisation. */ .remote-banner.external-banner { background: var(--warn-soft); border-color: var(--warn); } + +/* The line offering the whole folder once the page is selected. */ +.list-hint.select-all-hint { background: var(--accent-soft); color: var(--accent-soft-fg); } diff --git a/web/src/views/mail/MailView.tsx b/web/src/views/mail/MailView.tsx index beae613..b74a89c 100644 --- a/web/src/views/mail/MailView.tsx +++ b/web/src/views/mail/MailView.tsx @@ -105,6 +105,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; const emails = useMail((s) => s.emails); const threads = useMail((s) => s.threads); const selected = useMail((s) => s.selected); + const selectedAll = useMail((s) => s.selectedAll); const rowThreadId = useCallback((rowId: Id) => emails[rowId]?.threadId, [emails]); const currentRowIndex = useMemo(() => { @@ -118,7 +119,17 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; /** Email ids affected by an action on rows (selection or focused/open row). */ const targetIds = useCallback( - (rowIds?: Id[]): Id[] => { + async (rowIds?: Id[]): Promise => { + /* + * Everything the query matches, rather than the rows that happen to be + * loaded. Resolved from the server here and not expanded below: the + * uncollapsed query already returns every message, and the expansion + * below walks loaded Email objects, which is exactly what these are not. + * + * Only when the action came from the selection. An action aimed at one + * row -- a right-click, a swipe -- means that row, whatever is ticked. + */ + if (!rowIds && selectedAll) return await useMail.getState().queryAllIds(); const rows = rowIds ?? (Object.keys(selected).length ? Object.keys(selected) : focusId ? [focusId] : threadId ? ids.filter((id) => rowThreadId(id) === threadId) : []); const out = new Set(); for (const r of rows) { @@ -132,7 +143,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; } return [...out]; }, - [selected, focusId, threadId, ids, rowThreadId, emails, threads, list], + [selected, selectedAll, focusId, threadId, ids, rowThreadId, emails, threads, list], ); const afterAction = useCallback( @@ -189,13 +200,13 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; const actions = useMemo( () => ({ archive: async (rows?: Id[]) => { - const t = targetIds(rows); + const t = await targetIds(rows); if (!t.length) return; await useMail.getState().archive(t); afterAction(true); }, trash: async (rows?: Id[]) => { - const t = targetIds(rows); + const t = await targetIds(rows); if (!t.length) return; const mail = useMail.getState(); const trashId = mail.roleId("trash"); @@ -208,7 +219,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; afterAction(true); }, spam: async (rows?: Id[]) => { - const t = targetIds(rows); + const t = await targetIds(rows); if (!t.length) return; const mail = useMail.getState(); const junk = mail.roleId("junk"); @@ -217,20 +228,20 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; afterAction(true); }, read: async (read: boolean, rows?: Id[]) => { - const t = targetIds(rows); + const t = await targetIds(rows); if (t.length) await useMail.getState().markRead(t, read); useMail.getState().clearSelection(); }, star: async (on: boolean, rows?: Id[]) => { - const t = targetIds(rows); + const t = await targetIds(rows); if (t.length) await useMail.getState().star(t, on); }, - move: (rows?: Id[]) => { - const t = targetIds(rows); + move: async (rows?: Id[]) => { + const t = await targetIds(rows); if (t.length) setMovePicker({ ids: t }); }, - label: (rows: Id[] | undefined, anchor: { x: number; y: number }) => { - const t = targetIds(rows); + label: async (rows: Id[] | undefined, anchor: { x: number; y: number }) => { + const t = await targetIds(rows); if (t.length) setLabelPicker({ ids: t, anchor }); }, moveTo: async (ids: Id[], mailboxId: Id) => { @@ -276,7 +287,7 @@ export function MailView({ mailboxId, threadId, search }: { mailboxId?: string; { keys: "#", description: "Delete", group: "Actions", handler: () => void actions.trash() }, { keys: "delete", description: "", group: "Actions", handler: () => void actions.trash() }, { keys: "!", description: "Report spam / not spam", group: "Actions", handler: () => void actions.spam() }, - { keys: "s", description: "Star / unstar", group: "Actions", handler: () => { const t = targetIds(); const on = !t.every((id) => emails[id]?.keywords.$flagged); void actions.star(on); } }, + { keys: "s", description: "Star / unstar", group: "Actions", handler: () => { void (async () => { const t = await targetIds(); const on = !t.every((id) => emails[id]?.keywords.$flagged); void actions.star(on); })(); } }, { keys: "shift+i", description: "Mark as read", group: "Actions", handler: () => void actions.read(true) }, { keys: "shift+u", description: "Mark as unread", group: "Actions", handler: () => void actions.read(false) }, { keys: "v", description: "Move to…", group: "Actions", handler: () => actions.move() }, diff --git a/web/src/views/mail/MessageList.tsx b/web/src/views/mail/MessageList.tsx index 54fcd42..5c6ecfb 100644 --- a/web/src/views/mail/MessageList.tsx +++ b/web/src/views/mail/MessageList.tsx @@ -6,6 +6,7 @@ import { useMail, type ListState } from "@/store/mail"; import { dateTimeKey, useSettings } from "@/store/settings"; import type { Email, Id } from "@/jmap/types"; import { formatListDate } from "@/lib/format"; +import { mailboxDisplayName } from "@/lib/mailboxName"; import { groupByArchivePath, archivePath, type ArchiveGranularity } from "@/lib/archiveDate"; import { canEmpty, confirmAndEmpty, emptyLabel } from "@/lib/emptyFolder"; import { displayName, shortName } from "@/lib/address"; @@ -242,6 +243,8 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on [ctxTargets, emails], ); const allSelected = ids.length > 0 && ids.every((id) => selected[id]); + const selectedAll = useMail((st) => st.selectedAll); + const selectAllMatching = useMail((st) => st.selectAllMatching); const someUnread = ctxTargets.some((id) => !emails[id]?.keywords.$seen); const someUnstarred = ctxTargets.some((id) => !emails[id]?.keywords.$flagged); @@ -265,7 +268,7 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on /> {selCount > 0 ? ( <> - {plural(selCount, { one: "{n} selected", other: "{n} selected" })} + {plural(selectedAll ? (list?.total ?? selCount) : selCount, { one: "{n} selected", other: "{n} selected" })} @@ -350,6 +353,29 @@ export function MessageList({ title, list, openThreadId, focusId, setFocusId, on )} + {/* + The step past the checkbox. Ticking it selects the rows that are + loaded, which on a folder of ten thousand is fifty of them -- and a + checkbox that silently meant all ten thousand would be the worst of + both. So the wider selection is offered here, in a line that says what + each of the two actually covers, and taken deliberately. + */} + {allSelected && !selectedAll && (list?.total ?? 0) > ids.length && ( +
+ {t("All {n} on this page are selected.", { n: String(ids.length) })} + +
+ )} + {selectedAll && ( +
+ + {t("All {n} in {folder} are selected.", { n: String(list?.total ?? 0), folder: mailbox ? mailboxDisplayName(mailbox) : t("this view") })} + + +
+ )} {list?.error && (
{list.error}