Merge branch 'fix/move-picker-folder-order' into 'main'

List folders in sidebar order in the move-to picker

Closes #1

See merge request coffey-labs/ihasmail!7

web/src/lib/mailbox/__tests__/folderOrder.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { canPlaceFolder, compareFolders, neighbour, placeFolder, siblingsOf } from "../folderOrder";
+import { canPlaceFolder, compareFolders, neighbour, placeFolder, siblingsOf, treeOrder } from "../folderOrder";
 import type { Id, Mailbox } from "@/jmap/types";
 
 const RIGHTS = { mayRename: true, mayCreateChild: true } as Mailbox["myRights"];
@@ -119,3 +119,23 @@ describe("neighbour", () => {
     expect(neighbour(hidden, "alpha", "up", (m) => m.isSubscribed)).toEqual({ targetId: "junk", placement: "before" });
   });
 });
+
+describe("treeOrder", () => {
+  const ids = (all: Record<Id, Mailbox>) => treeOrder(all).map((m) => m.id);
+
+  it("lists the tree the way the sidebar does, each folder followed by its subfolders", () => {
+    expect(ids(fresh)).toEqual(["inbox", "drafts", "sent", "junk", "trash", "alpha", "work", "clients", "zeta"]);
+  });
+
+  it("follows a saved order rather than A–Z", () => {
+    // #1 on GitLab: the move-to picker kept the old order after the sidebar changed.
+    const ordered = apply(fresh, { zeta: { sortOrder: 10 }, sent: { sortOrder: 20 }, alpha: { sortOrder: 30 }, drafts: { sortOrder: 40 }, junk: { sortOrder: 50 }, trash: { sortOrder: 60 }, work: { sortOrder: 70 } });
+    expect(ids(ordered)).toEqual(["inbox", "zeta", "sent", "alpha", "drafts", "junk", "trash", "work", "clients"]);
+  });
+
+  it("still lists a folder the walk from the top can't reach", () => {
+    const looped = apply(fresh, { work: { parentId: "clients" } });
+    expect(ids(looped)).toHaveLength(Object.keys(looped).length);
+    expect(ids(looped)).toEqual(expect.arrayContaining(["work", "clients"]));
+  });
+});

web/src/lib/mailbox/folderOrder.ts

@@ -25,6 +25,37 @@ function roleRank(m: Mailbox): number {
   return m.role && m.role in ROLE_ORDER ? ROLE_ORDER[m.role]! : Number.MAX_SAFE_INTEGER;
 }
 
+/**
+ * Every folder, parents before their children and siblings in
+ * `compareFolders` order: the sidebar's order with every folder expanded.
+ * Lists that show all folders at once, like the move-to picker, use this so a
+ * folder sits where the user dragged it rather than where A–Z would put it.
+ *
+ * A folder the walk from the top never reaches (a parent loop the server
+ * should not allow) is appended rather than dropped, so it can still be
+ * picked.
+ */
+export function treeOrder(mailboxes: Record<Id, Mailbox>): Mailbox[] {
+  const byParent = new Map<Id | null, Mailbox[]>();
+  for (const m of Object.values(mailboxes)) {
+    const p = m.parentId && mailboxes[m.parentId] ? m.parentId : null;
+    byParent.set(p, [...(byParent.get(p) ?? []), m]);
+  }
+  for (const list of byParent.values()) list.sort(compareFolders);
+  const out: Mailbox[] = [];
+  const seen = new Set<Id>();
+  const walk = (parent: Id | null) => {
+    for (const m of byParent.get(parent) ?? []) {
+      if (seen.has(m.id)) continue;
+      seen.add(m.id);
+      out.push(m);
+      walk(m.id);
+    }
+  };
+  walk(null);
+  return out.concat(Object.values(mailboxes).filter((m) => !seen.has(m.id)).sort(compareFolders));
+}
+
 /** Every folder under `parentId` (null: the top level), in list order. */
 export function siblingsOf(mailboxes: Record<Id, Mailbox>, parentId: Id | null): Mailbox[] {
   return Object.values(mailboxes)

web/src/views/mail/MailboxPicker.tsx

@@ -5,6 +5,7 @@ import { Dialog } from "@/ui/dialog";
 import type { Id, Mailbox } from "@/jmap/types";
 import { t } from "@/lib/i18n";
 import { mailboxDisplayPath } from "@/lib/mailbox/mailboxName";
+import { treeOrder } from "@/lib/mailbox/folderOrder";
 
 /**
  * @param need which right a folder has to grant to be worth offering.
@@ -24,10 +25,11 @@ export function MailboxPicker({ title, onClose, onPick, exclude, need = "mayAddI
   const [q, setQ] = useState("");
   const [active, setActive] = useState(0);
   const list = useMemo(() => {
-    const all = Object.values(mailboxes)
+    // The sidebar's order, not A–Z by path: a folder dragged into place has to
+    // be found in the same place here.
+    const all = treeOrder(mailboxes)
       .filter((m) => !exclude?.includes(m.id) && m.myRights[need] && (!allow || allow(m.id)))
-      .map((m) => ({ m, path: mailboxDisplayPath(m, mailboxes), pick: () => onPick(m.id) }))
-      .sort((a, b) => (a.m.role === "inbox" ? -1 : b.m.role === "inbox" ? 1 : a.path.localeCompare(b.path)));
+      .map((m) => ({ m, path: mailboxDisplayPath(m, mailboxes), pick: () => onPick(m.id) }));
     const rows: { m: Mailbox | null; path: string; pick: () => void }[] = root ? [{ m: null, path: root.label, pick: root.onPick }, ...all] : all;
     const ql = q.trim().toLowerCase();
     return ql ? rows.filter((x) => x.path.toLowerCase().includes(ql)) : rows;

web/src/views/mail/__tests__/move-picker-order.test.tsx

@@ -0,0 +1,62 @@
+import { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+import { afterEach, beforeEach, describe, expect, it } from "vitest";
+import { MailboxPicker } from "../MailboxPicker";
+import { useMail } from "@/store/mail";
+import type { Mailbox, MailboxRole } from "@/jmap/types";
+
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+/**
+ * The move-to picker (v) lists folders in the sidebar's order (#1 on GitLab).
+ *
+ * It used to sort A–Z by path, so a folder dragged into place in the sidebar
+ * turned up somewhere else here. The ordering has its own tests in
+ * lib/mailbox; these check what the dialog actually shows.
+ */
+
+window.matchMedia = ((q: string) => ({ matches: false, media: q, addEventListener() {}, removeEventListener() {} })) as unknown as typeof window.matchMedia;
+
+const rights = { mayReadItems: true, mayAddItems: true, mayRemoveItems: true, maySetSeen: true, maySetKeywords: true, mayCreateChild: true, mayRename: true, mayDelete: true, maySubmit: true };
+const box = (id: string, name: string, parentId: string | null, role: MailboxRole = null, sortOrder = 0): Mailbox => ({
+  id, name, parentId, role, sortOrder, totalEmails: 0, unreadEmails: 0, totalThreads: 0, unreadThreads: 0, myRights: rights, isSubscribed: true,
+});
+
+/** Ordered by hand in the sidebar: Zeta dragged to the top, Alpha to the bottom. */
+const MAILBOXES = {
+  inbox: box("inbox", "Inbox", null, "inbox", 10),
+  zeta: box("zeta", "Zeta", null, null, 20),
+  sent: box("sent", "Sent", null, "sent", 30),
+  work: box("work", "Work", null, null, 40),
+  clients: box("clients", "Clients", "work"),
+  trash: box("trash", "Deleted Items", null, "trash", 50),
+  alpha: box("alpha", "Alpha", null, null, 60),
+};
+
+describe("the move-to picker", () => {
+  let host: HTMLDivElement;
+  let root: Root;
+  const rows = () => Array.from(document.querySelectorAll('[role="option"]')).map((r) => r.querySelector(".grow")?.textContent);
+
+  function open(props: Partial<Parameters<typeof MailboxPicker>[0]> = {}) {
+    act(() => root.render(<MailboxPicker title="Move to…" onClose={() => {}} onPick={() => {}} {...props} />));
+  }
+
+  beforeEach(() => {
+    useMail.setState({ mailboxes: MAILBOXES, mailboxesLoaded: true });
+    host = document.createElement("div");
+    document.body.appendChild(host);
+    root = createRoot(host);
+  });
+  afterEach(() => { act(() => root.unmount()); host.remove(); });
+
+  it("lists folders in the order they were dragged into, not A–Z", () => {
+    open();
+    expect(rows()).toEqual(["Inbox", "Zeta", "Sent", "Work", "Work / Clients", "Deleted Items", "Alpha"]);
+  });
+
+  it("keeps that order for the folders left after excluding one", () => {
+    open({ exclude: ["work"] });
+    expect(rows()).toEqual(["Inbox", "Zeta", "Sent", "Work / Clients", "Deleted Items", "Alpha"]);
+  });
+});