Select a whole folder, not just the rows that are loaded
The header checkbox selected the loaded page. On a folder of ten thousand that is fifty of them, and the only way to act on the rest was to scroll until they loaded and tick again. A line now offers the rest by name once the page is selected, 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 rather than 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 and Email/set refuses more ids than maxObjectsInSet in one go -- which setEmails already chunks for. It resolves uncollapsed, unlike the list: "everything in this folder" means every message rather than one per thread, and expanding threads the way a click does is impossible here anyway, since that walks loaded Email objects and these are the ones that were never loaded. Two things this exposed. Undo is now withheld once a move reaches messages that were never loaded. It restores the folders each message was in, taken from what the browser holds, and for an unloaded message that is nothing -- so the undo would have written an empty mailboxIds and left the message in no folder at all, which is worse than the move it was undoing. move() and archiveByDate() both did this; both now drop the offer rather than restore something wrong. And an action consumes the wider selection. The optimistic paths cleared the selected ids but not the flag, so the next action would have silently reached the whole folder again.
This commit is contained in:
@@ -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<Record<string, unknown>> = [];
|
||||
|
||||
const fetchMock = vi.fn(async (_url: string, init: RequestInit) => {
|
||||
const body = JSON.parse(init.body as string) as { methodCalls: [string, Record<string, unknown>, 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<string, unknown>);
|
||||
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<string, boolean> }).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({});
|
||||
});
|
||||
});
|
||||
+82
-7
@@ -130,6 +130,12 @@ export interface MailState {
|
||||
vacation: VacationResponse | null;
|
||||
list: ListState | null;
|
||||
selected: Record<Id, true>;
|
||||
/**
|
||||
* 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<Id, true>;
|
||||
lastSeenInboxEmailIds: Id[] | null;
|
||||
@@ -185,6 +191,10 @@ export interface MailState {
|
||||
select(ids: Id[], on: boolean): void;
|
||||
clearSelection(): void;
|
||||
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<Id[]>;
|
||||
setAnchor(id: Id | null): void;
|
||||
|
||||
applyChanges(types: Set<string>): Promise<void>;
|
||||
@@ -211,6 +221,7 @@ export const useMail = create<MailState>((set, get) => ({
|
||||
vacation: null,
|
||||
list: null,
|
||||
selected: {},
|
||||
selectedAll: false,
|
||||
anchorId: null,
|
||||
loadingThreads: {},
|
||||
lastSeenInboxEmailIds: null,
|
||||
@@ -236,6 +247,7 @@ export const useMail = create<MailState>((set, get) => ({
|
||||
vacation: null,
|
||||
list: null,
|
||||
selected: {},
|
||||
selectedAll: false,
|
||||
anchorId: null,
|
||||
lastSeenInboxEmailIds: null,
|
||||
});
|
||||
@@ -287,6 +299,7 @@ export const useMail = create<MailState>((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 {
|
||||
@@ -466,8 +479,18 @@ export const useMail = create<MailState>((set, get) => ({
|
||||
const { emails, mailboxes } = get();
|
||||
const prev: Record<Id, Record<Id, boolean>> = {};
|
||||
const update: Record<Id, Record<string, unknown>> = {};
|
||||
/*
|
||||
* 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 } };
|
||||
}
|
||||
@@ -475,7 +498,7 @@ export const useMail = create<MailState>((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 {
|
||||
@@ -490,7 +513,7 @@ export const useMail = create<MailState>((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<Id, Record<string, unknown>> = {};
|
||||
@@ -558,7 +581,7 @@ export const useMail = create<MailState>((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);
|
||||
@@ -595,7 +618,13 @@ export const useMail = create<MailState>((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<Id, Record<Id, boolean>> = {};
|
||||
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 {
|
||||
@@ -621,7 +650,7 @@ export const useMail = create<MailState>((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<Id, Record<string, unknown>> = {};
|
||||
@@ -891,14 +920,60 @@ export const useMail = create<MailState>((set, get) => ({
|
||||
});
|
||||
},
|
||||
clearSelection() {
|
||||
set({ selected: {} });
|
||||
set({ selected: {}, selectedAll: false });
|
||||
},
|
||||
selectAll() {
|
||||
const l = get().list;
|
||||
if (!l) return;
|
||||
const next: Record<Id, true> = {};
|
||||
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<QueryResponse>("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 });
|
||||
|
||||
Reference in New Issue
Block a user