Let a filter rule be dragged into place

Twenty-five rules and two buttons that move one place at a time meant a
rule pushed to the wrong end cost ten clicks to bring back. It can now
be dragged.

A grip on the left of each card arms the drag, so the switch, the name
and the buttons still take a plain click, and the up and down buttons
stay for the keyboard. The card being dragged fades; the one under the
pointer draws a line on the edge the rule would land on, top half or
bottom.

The guard against dropping a rule onto itself reads a ref rather than
state: dragstart and the first dragover can arrive in the same frame,
and a stale read there drew a drop line on the card being dragged. Found
by driving the real thing in a browser, and covered by a test that fires
the two events back to back.
This commit is contained in:
2026-08-25 08:51:28 -07:00
parent b4fd3d3ab4
commit 696b3713ed
5 changed files with 184 additions and 5 deletions
+19 -1
View File
@@ -1,5 +1,5 @@
import { describe, expect, it } from "vitest"; import { describe, expect, it } from "vitest";
import { newRule, rulesToSieve, sieveToRules, testToSieve, sieveString, upsertRule } from "../sieve"; import { newRule, reorderRules, rulesToSieve, sieveToRules, testToSieve, sieveString, upsertRule, type SieveRule } from "../sieve";
describe("sieve codec", () => { describe("sieve codec", () => {
it("escapes strings", () => { it("escapes strings", () => {
@@ -37,3 +37,21 @@ describe("sieve codec", () => {
expect(sieveToRules("")).toEqual([]); expect(sieveToRules("")).toEqual([]);
}); });
}); });
describe("reordering rules", () => {
const ids = (rs: SieveRule[]) => rs.map((r) => r.id);
const list = ["a", "b", "c", "d"].map((id) => newRule({ id }));
it("drops a rule above or below the card it was dropped on", () => {
expect(ids(reorderRules(list, "a", "c", false))).toEqual(["b", "a", "c", "d"]);
expect(ids(reorderRules(list, "a", "c", true))).toEqual(["b", "c", "a", "d"]);
expect(ids(reorderRules(list, "d", "a", false))).toEqual(["d", "a", "b", "c"]);
expect(ids(reorderRules(list, "b", "d", true))).toEqual(["a", "c", "d", "b"]);
});
it("leaves the list alone when the drop goes nowhere", () => {
expect(reorderRules(list, "a", "a", true)).toBe(list);
expect(reorderRules(list, "a", "zz", true)).toBe(list);
expect(reorderRules(list, "zz", "a", true)).toBe(list);
expect(ids(list)).toEqual(["a", "b", "c", "d"]);
});
});
+14
View File
@@ -210,6 +210,20 @@ export function upsertRule(rules: SieveRule[], rule: SieveRule): SieveRule[] {
return rules.some((x) => x.id === rule.id) ? rules.map((x) => (x.id === rule.id ? rule : x)) : [...rules, rule]; return rules.some((x) => x.id === rule.id) ? rules.map((x) => (x.id === rule.id ? rule : x)) : [...rules, rule];
} }
/**
* Moves the rule `fromId` to sit either side of `toId`. `below` says which,
* decided by which half of the target card the pointer was over.
*/
export function reorderRules(rules: SieveRule[], fromId: string, toId: string, below: boolean): SieveRule[] {
if (fromId === toId) return rules;
const moved = rules.find((r) => r.id === fromId);
const rest = rules.filter((r) => r.id !== fromId);
const target = rest.findIndex((r) => r.id === toId);
if (!moved || target < 0) return rules;
const at = below ? target + 1 : target;
return [...rest.slice(0, at), moved, ...rest.slice(at)];
}
export function describeRule(r: SieveRule): string { export function describeRule(r: SieveRule): string {
const tests = r.tests const tests = r.tests
.map((t) => { .map((t) => {
+6
View File
@@ -605,6 +605,12 @@ img { max-width: 100%; }
.swatch.active { border-color: var(--fg); } .swatch.active { border-color: var(--fg); }
.rule-card { border: 1px solid var(--border); border-radius: var(--radius); padding: 12px 14px; margin-bottom: 10px; background: var(--bg-elev); } .rule-card { border: 1px solid var(--border); border-radius: var(--radius); padding: 12px 14px; margin-bottom: 10px; background: var(--bg-elev); }
.rule-card.disabled { opacity: .6; } .rule-card.disabled { opacity: .6; }
.rule-card.dragging { opacity: .5; }
/* The line shows which side of this card the dragged rule would land on. */
.rule-card.drop-above { box-shadow: inset 0 3px 0 0 var(--accent); }
.rule-card.drop-below { box-shadow: inset 0 -3px 0 0 var(--accent); }
.rule-card .drag-handle { display: flex; align-items: center; padding: 2px; margin-left: -4px; color: var(--fg-faint); cursor: grab; touch-action: none; }
.rule-card .drag-handle:active { cursor: grabbing; }
.rule-row { display: grid; grid-template-columns: 1fr 1fr 1fr auto; gap: 8px; align-items: center; margin-bottom: 8px; } .rule-row { display: grid; grid-template-columns: 1fr 1fr 1fr auto; gap: 8px; align-items: center; margin-bottom: 8px; }
.rule-row.actions { grid-template-columns: 1fr 2fr auto; } .rule-row.actions { grid-template-columns: 1fr 2fr auto; }
/* A header typed by hand needs a box of its own, alongside the comparator. */ /* A header typed by hand needs a box of its own, alongside the comparator. */
+47 -4
View File
@@ -1,8 +1,8 @@
import { useEffect, useState } from "react"; import { useEffect, useRef, useState } from "react";
import { ArrowDown, ArrowUp, Code, Plus, Trash2, Wand2, Play, AlertTriangle, Power } from "lucide-react"; import { ArrowDown, ArrowUp, Code, GripVertical, Plus, Trash2, Wand2, Play, AlertTriangle, Power } from "lucide-react";
import { useSieve } from "@/store/sieve"; import { useSieve } from "@/store/sieve";
import { useMail } from "@/store/mail"; import { useMail } from "@/store/mail";
import { describeRule, newRule, rulesToSieve, upsertRule, type SieveRule } from "@/lib/sieve"; import { describeRule, newRule, reorderRules, rulesToSieve, upsertRule, type SieveRule } from "@/lib/sieve";
import { RuleDialog } from "./RuleDialog"; import { RuleDialog } from "./RuleDialog";
import { saveAndApply } from "../mail/FilterFromMessage"; import { saveAndApply } from "../mail/FilterFromMessage";
import { confirmDialog, promptDialog } from "@/ui/dialog"; import { confirmDialog, promptDialog } from "@/ui/dialog";
@@ -40,6 +40,9 @@ export function FiltersSettings() {
); );
} }
/** Private drag type, so a rule can only be dropped on the rule list. */
const RULE_MIME = "application/x-ihasmail-sieve-rule";
function RulesEditor() { function RulesEditor() {
const sieve = useSieve(); const sieve = useSieve();
const { script, rules, content } = sieve.rules(); const { script, rules, content } = sieve.rules();
@@ -49,6 +52,19 @@ function RulesEditor() {
const list = local ?? rules ?? []; const list = local ?? rules ?? [];
const dirty = local !== null; const dirty = local !== null;
const inbox = useMail((s) => { const id = s.roleId("inbox"); return id ? s.mailboxes[id] : undefined; }); const inbox = useMail((s) => { const id = s.roleId("inbox"); return id ? s.mailboxes[id] : undefined; });
/** The rule being dragged, the one armed to be, and where a drop would land. */
const [dragId, setDragId] = useState<string | null>(null);
const [armed, setArmed] = useState<string | null>(null);
const [over, setOver] = useState<{ id: string; below: boolean } | null>(null);
/**
* The same id as `dragId`, kept synchronously: the first dragover can arrive
* before React has re-rendered with the state, and a stale read there draws a
* drop line on the card being dragged.
*/
const dragging = useRef<string | null>(null);
const endDrag = () => { dragging.current = null; setDragId(null); setArmed(null); setOver(null); };
/** Which half of the card the pointer is over decides which side of it the rule lands. */
const isBelow = (el: HTMLElement, y: number) => { const b = el.getBoundingClientRect(); return y > b.top + b.height / 2; };
const activeIsOther = script && script.name !== "ihasmail" && script.isActive; const activeIsOther = script && script.name !== "ihasmail" && script.isActive;
const save = async (next: SieveRule[]) => { const save = async (next: SieveRule[]) => {
@@ -79,8 +95,35 @@ function RulesEditor() {
{activeIsOther && <div className="warn-box mb-16">Another script ({script?.name}) is active. Saving rules here will activate the ihasmail script instead.</div>} {activeIsOther && <div className="warn-box mb-16">Another script ({script?.name}) is active. Saving rules here will activate the ihasmail script instead.</div>}
{list.length === 0 && <div className="empty" style={{ padding: 32 }}><Wand2 size={32} /><h3>No filters yet</h3><p>Create a rule to move newsletters to a folder, flag important senders, or forward mail.</p></div>} {list.length === 0 && <div className="empty" style={{ padding: 32 }}><Wand2 size={32} /><h3>No filters yet</h3><p>Create a rule to move newsletters to a folder, flag important senders, or forward mail.</p></div>}
{list.map((r, i) => ( {list.map((r, i) => (
<div key={r.id} className={`rule-card ${r.enabled ? "" : "disabled"}`}> <div
key={r.id}
className={`rule-card ${r.enabled ? "" : "disabled"} ${dragId === r.id ? "dragging" : ""} ${over?.id === r.id ? (over.below ? "drop-below" : "drop-above") : ""}`}
draggable={armed === r.id}
onDragStart={(e) => { e.dataTransfer.setData(RULE_MIME, r.id); e.dataTransfer.effectAllowed = "move"; dragging.current = r.id; setDragId(r.id); }}
onDragEnd={endDrag}
onDragOver={(e) => {
if (!e.dataTransfer.types.includes(RULE_MIME) || dragging.current === r.id) return;
e.preventDefault();
e.dataTransfer.dropEffect = "move";
const below = isBelow(e.currentTarget, e.clientY);
if (over?.id !== r.id || over.below !== below) setOver({ id: r.id, below });
}}
onDragLeave={() => setOver((o) => (o?.id === r.id ? null : o))}
onDrop={(e) => {
e.preventDefault();
const from = e.dataTransfer.getData(RULE_MIME);
if (from) setLocal(reorderRules(list, from, r.id, isBelow(e.currentTarget, e.clientY)));
endDrag();
}}
>
<div className="row"> <div className="row">
<span
className="drag-handle"
title="Drag to reorder"
aria-hidden="true"
onPointerDown={() => setArmed(r.id)}
onPointerUp={() => setArmed(null)}
><GripVertical size={16} /></span>
<Switch checked={r.enabled} onChange={(v) => setLocal(list.map((x) => (x.id === r.id ? { ...x, enabled: v } : x)))} /> <Switch checked={r.enabled} onChange={(v) => setLocal(list.map((x) => (x.id === r.id ? { ...x, enabled: v } : x)))} />
<div className="grow" style={{ cursor: "pointer", minWidth: 0 }} onClick={() => setEditing(r)}> <div className="grow" style={{ cursor: "pointer", minWidth: 0 }} onClick={() => setEditing(r)}>
<div style={{ fontWeight: 600 }}>{r.name}</div> <div style={{ fontWeight: 600 }}>{r.name}</div>
@@ -0,0 +1,98 @@
import { act } from "react";
import { createRoot, type Root } from "react-dom/client";
import { afterEach, beforeEach, describe, expect, it } from "vitest";
import { FiltersSettings } from "../FiltersSettings";
import { useSieve } from "@/store/sieve";
import { newRule, rulesToSieve } from "@/lib/sieve";
(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
/** jsdom has no DataTransfer, and no layout — both are faked just enough. */
function dataTransfer() {
const data: Record<string, string> = {};
return {
types: [] as string[],
effectAllowed: "",
dropEffect: "",
setData(k: string, v: string) { data[k] = v; this.types.push(k); },
getData(k: string) { return data[k] ?? ""; },
};
}
function fire(el: Element, type: string, dt: ReturnType<typeof dataTransfer>, clientY = 0) {
const ev = new Event(type, { bubbles: true, cancelable: true });
Object.defineProperty(ev, "dataTransfer", { value: dt });
Object.defineProperty(ev, "clientY", { value: clientY });
act(() => { el.dispatchEvent(ev); });
}
/** Cards have no size in jsdom, so any positive clientY counts as the lower half. */
const LOWER = 1;
const UPPER = 0;
describe("reordering rules by dragging", () => {
let host: HTMLDivElement;
let root: Root;
const cards = () => Array.from(document.querySelectorAll(".rule-card"));
const names = () => cards().map((c) => c.querySelector('div[style*="font-weight"]')?.textContent);
const press = (i: number) => act(() => { cards()[i]!.querySelector(".drag-handle")!.dispatchEvent(new Event("pointerdown", { bubbles: true })); });
beforeEach(() => {
const rules = ["Newsletters", "From the boss", "Receipts"].map((name, i) => newRule({ id: `r${i}`, name }));
useSieve.setState({
accountId: "a", available: true, loading: false, error: null,
scripts: [{ id: "s1", name: "ihasmail", blobId: "b1", isActive: true }],
contents: { s1: rulesToSieve(rules) },
});
host = document.createElement("div");
document.body.appendChild(host);
root = createRoot(host);
act(() => root.render(<FiltersSettings />));
});
afterEach(() => { act(() => root.unmount()); host.remove(); });
it("arms dragging only from the handle of the rule pressed", () => {
expect(cards()).toHaveLength(3);
expect(cards().map((c) => c.getAttribute("draggable"))).toEqual(["false", "false", "false"]);
press(1);
expect(cards().map((c) => c.getAttribute("draggable"))).toEqual(["false", "true", "false"]);
});
it("drops a rule below the card it was dragged onto", () => {
press(0);
const dt = dataTransfer();
fire(cards()[0]!, "dragstart", dt);
expect(cards()[0]!.className).toContain("dragging");
fire(cards()[2]!, "dragover", dt, LOWER);
expect(cards()[2]!.className).toContain("drop-below");
fire(cards()[2]!, "drop", dt, LOWER);
expect(names()).toEqual(["From the boss", "Receipts", "Newsletters"]);
expect(cards().every((c) => !/dragging|drop-(above|below)/.test(c.className))).toBe(true);
});
it("drops a rule above the card when the pointer is in its top half", () => {
press(2);
const dt = dataTransfer();
fire(cards()[2]!, "dragstart", dt);
fire(cards()[0]!, "dragover", dt, UPPER);
expect(cards()[0]!.className).toContain("drop-above");
fire(cards()[0]!, "drop", dt, UPPER);
expect(names()).toEqual(["Receipts", "Newsletters", "From the boss"]);
});
it("draws no drop line on the rule being dragged, even before React re-renders", () => {
press(1);
const dt = dataTransfer();
// dragstart and dragover back to back: the guard cannot wait for a render.
fire(cards()[1]!, "dragstart", dt);
fire(cards()[1]!, "dragover", dt, LOWER);
expect(cards().some((c) => /drop-(above|below)/.test(c.className))).toBe(false);
fire(cards()[1]!, "drop", dt, LOWER);
expect(names()).toEqual(["Newsletters", "From the boss", "Receipts"]);
});
it("ignores a drag that is not a rule", () => {
const dt = dataTransfer();
dt.setData("text/plain", "hello");
fire(cards()[1]!, "dragover", dt, LOWER);
expect(cards().some((c) => /drop-(above|below)/.test(c.className))).toBe(false);
});
});