Merge pull request #177 from Coffey-Labs/fix/sieve-unsaved-changes

Ask before the filter editors lose your changes

web/src/App.tsx

@@ -1,5 +1,5 @@
 import { Fragment, lazy, Suspense, useEffect, useState } from "react";
-import { Route, Switch, Redirect, useLocation } from "wouter";
+import { Route, Switch, Redirect, useLocation, Router } from "wouter";
 import { useSession } from "@/store/session";
 import { useMail } from "@/store/mail";
 import { scheduleSupported, useScheduled } from "@/store/scheduled";
@@ -21,6 +21,7 @@ import { PAINTED_FROM_CACHE, useSettings, syncedPart } from "@/store/settings";
 import { armSettingsSync, loadRemoteSettings, queueSettingsPush, settingsAlreadyLoadedFor, settingsSyncAvailable } from "@/lib/settingsSync";
 import { listenForVerification, renewWebPush } from "@/lib/webpushEnable";
 import { useLanguageVersion, whenLanguageReady } from "@/lib/i18n";
+import { confirmLeaveUnsaved, hasUnsavedChanges } from "@/lib/unsavedChanges";
 
 const ContactsView = lazy(() => import("@/views/contacts/ContactsView").then((m) => ({ default: m.ContactsView })));
 const CalendarView = lazy(() => import("@/views/calendar/CalendarView").then((m) => ({ default: m.CalendarView })));
@@ -70,11 +71,32 @@ export function App() {
     );
   }
   return (
-    <>
+    /*
+     * Every in-app navigation runs through `aroundNav` -- links, redirects and
+     * `navigate()` alike, since wouter routes them all through the same place.
+     * That is what makes the guard hold for the app rail and the settings nav
+     * without either of them knowing an editor exists.
+     *
+     * The back button is the gap: by the time `popstate` arrives the history
+     * has already moved, and the only way to hold the page would be to push an
+     * entry back, which breaks the button for everyone who has nothing pending.
+     * Reload and tab close are covered by `beforeunload` instead.
+     */
+    <Router
+      aroundNav={(navigate, to, options) => {
+        if (!hasUnsavedChanges()) {
+          navigate(to, options);
+          return;
+        }
+        void confirmLeaveUnsaved().then((ok) => {
+          if (ok) navigate(to, options);
+        });
+      }}
+    >
       <Fragment key={languageVersion}>{status === "anonymous" ? <LoginPage /> : <AuthedApp />}</Fragment>
       <ToastHost />
       <ConfirmHost />
-    </>
+    </Router>
   );
 }
 

web/src/lib/unsavedChanges.ts

@@ -0,0 +1,107 @@
+import { useEffect, useRef } from "react";
+import { choiceDialog } from "@/ui/dialog";
+import { t } from "@/lib/i18n";
+
+/**
+ * Editors that would lose work if you walked away from them.
+ *
+ * The filter editors keep their edits in component state, so every way out of
+ * the page -- a settings link, the app rail, the Rules/Scripts switch -- threw
+ * them away without a word, and the only sign there had been anything to lose
+ * was a Save button that a screenful of rules had already pushed past the
+ * bottom of the window. That is issue #175.
+ *
+ * An editor registers what it has pending here; navigation asks before it
+ * happens. The question is deliberately not a yes/no: "leave without saving?"
+ * makes losing the work the easy answer and saving it the one you have to back
+ * out and find, when saving is what almost everybody wants.
+ */
+export interface UnsavedChanges {
+  /** Whether anything would be lost by leaving right now. */
+  dirty: boolean;
+  /**
+   * Persist the edits. `false` keeps you where you are: a save that failed is
+   * exactly the moment not to navigate, because the edits only exist here.
+   */
+  save: () => Promise<boolean>;
+  /** Throw the edits away. */
+  discard: () => void;
+  /** Already translated, and specific: it says what is about to be lost. */
+  message: string;
+}
+
+/**
+ * Held by reference rather than by value, so the callbacks the dialog runs are
+ * the ones from the editor's latest render and not from whenever it mounted.
+ */
+type Slot = { current: UnsavedChanges };
+
+const slots = new Set<Slot>();
+
+/** Register this editor's pending edits for as long as it is on screen. */
+export function useUnsavedChanges(changes: UnsavedChanges): void {
+  const slot = useRef(changes);
+  slot.current = changes;
+  useEffect(() => {
+    slots.add(slot);
+    return () => {
+      slots.delete(slot);
+    };
+  }, []);
+  /*
+   * Reloading and closing the tab are the browser's to ask about, and all it
+   * will show is its own generic wording -- custom text was removed years ago.
+   * Generic is still better than silent, and it costs one listener, armed only
+   * while there is something to lose.
+   */
+  useEffect(() => {
+    if (!changes.dirty) return;
+    const warn = (e: BeforeUnloadEvent) => {
+      e.preventDefault();
+      e.returnValue = "";
+    };
+    window.addEventListener("beforeunload", warn);
+    return () => window.removeEventListener("beforeunload", warn);
+  }, [changes.dirty]);
+}
+
+/** Whether leaving now would lose anything. Cheap enough for every navigation. */
+export function hasUnsavedChanges(): boolean {
+  for (const slot of slots) if (slot.current.dirty) return true;
+  return false;
+}
+
+/**
+ * Ask about anything pending. Resolves true when it is safe to go.
+ *
+ * Dismissing the dialog -- Escape, the backdrop, the close button -- is not
+ * one of the answers, so it cannot be mistaken for "discard": it leaves you on
+ * the page with the edits intact.
+ */
+export async function confirmLeaveUnsaved(): Promise<boolean> {
+  /*
+   * A snapshot, not the live set. `save` and `discard` both make an editor
+   * clean, but not until React has re-rendered it, and re-reading `slots`
+   * between questions would find the same editor still dirty and ask twice.
+   */
+  for (const slot of [...slots]) {
+    const pending = slot.current;
+    if (!pending.dirty) continue;
+    const answer = await choiceDialog({
+      title: t("Save your changes?"),
+      message: pending.message,
+      choices: [
+        { value: "save", label: t("Save changes") },
+        { value: "discard", label: t("Discard changes"), hint: t("What you changed here will be lost."), danger: true },
+      ],
+      cancelLabel: t("Stay here"),
+    });
+    if (answer === null) return false;
+    if (answer === "discard") {
+      pending.discard();
+      continue;
+    }
+    if (!(await pending.save())) return false;
+  }
+  return true;
+}

web/src/styles/app.css

@@ -730,7 +730,7 @@ select optgroup { background-color: var(--bg-elev); color: var(--fg); }
 .settings-layout { display: grid; grid-template-columns: 240px 1fr; height: 100%; min-height: 0; flex: 1; }
 .settings-nav { border-right: 1px solid var(--border); padding: 12px 8px; overflow-y: auto; }
 .settings-nav .nav-item { margin-right: 0; border-radius: var(--radius-sm); }
-.settings-content { overflow-y: auto; padding: 24px 32px 64px; max-width: 860px; }
+.settings-content { --pad-b: 64px; overflow-y: auto; padding: 24px 32px var(--pad-b); max-width: 860px; }
 .settings-content h1 { margin: 0 0 4px; font-size: 1.5em; font-weight: 650; }
 .settings-content h2 { margin: 28px 0 12px; font-size: 1.05em; font-weight: 650; padding-bottom: 6px; border-bottom: 1px solid var(--border); }
 .settings-content .lead { color: var(--fg-muted); margin: 0 0 20px; }
@@ -754,6 +754,18 @@ select optgroup { background-color: var(--bg-elev); color: var(--fg); }
 .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; }
+
+/* The bar that says whether there is unsaved work, kept where it can be read.
+   Unpinned it sat after the rules, which on a full list means off the bottom of
+   the window: the one indicator that anything was pending was the one thing you
+   had to scroll to find (issue #175).
+
+   The offset is what pins it flush. A sticky `bottom: 0` stops at the scroll
+   container's padding box, which leaves the pane's bottom padding as a strip
+   below the bar for rules to scroll through; sticking to minus that padding
+   closes it, on the narrow layout's larger padding as well. */
+.save-bar { position: sticky; bottom: calc(var(--pad-b, 0px) * -1); z-index: 2; margin-top: 12px; padding: 12px 0; background: var(--bg); border-top: 1px solid var(--border); }
+.save-bar .unsaved { color: var(--warn); font-weight: 600; font-size: .9em; }
 .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; }
 /* A header typed by hand needs a box of its own, alongside the comparator. */
@@ -1017,7 +1029,7 @@ select optgroup { background-color: var(--bg-elev); color: var(--fg); }
   .settings-layout.section .settings-nav { display: none; }
   .settings-layout.root .settings-nav { display: block; border-right: 0; }
   .settings-layout.root .settings-content { display: none; }
-  .settings-content { padding: 16px 16px 80px; }
+  .settings-content { --pad-b: 80px; padding: 16px 16px var(--pad-b); }
   .contacts-layout { grid-template-columns: 1fr; }
   .contacts-books { display: none; }
   .contacts-layout.detail .contacts-list { display: none; }

web/src/views/settings/FiltersSettings.tsx

@@ -10,10 +10,22 @@ import { Switch, Spinner } from "@/ui/misc";
 import { toast } from "@/ui/toast";
 import type { SieveScript } from "@/jmap/types";
 import { t, tNode } from "@/lib/i18n";
+import { confirmLeaveUnsaved, useUnsavedChanges } from "@/lib/unsavedChanges";
 
 export function FiltersSettings() {
   const sieve = useSieve();
   const [tab, setTab] = useState<"rules" | "scripts">("rules");
+  /*
+   * Switching tabs unmounts the editor you were in, which is a way of losing
+   * work that never leaves the page and so never reaches the router's guard.
+   * Ask here for the same reason navigation asks.
+   */
+  const switchTab = (next: "rules" | "scripts") => {
+    if (next === tab) return;
+    void confirmLeaveUnsaved().then((ok) => {
+      if (ok) setTab(next);
+    });
+  };
   useEffect(() => {
     if (sieve.available && !sieve.scripts.length && !sieve.loading) void sieve.load();
     // eslint-disable-next-line react-hooks/exhaustive-deps
@@ -33,8 +45,8 @@ export function FiltersSettings() {
       <h1>{t("Filters & rules")}</h1>
       <p className="lead">{t("Sort incoming mail automatically. Rules run on the server (Sieve), so they work for every client you use.")}</p>
       <div className="view-switch" style={{ marginBottom: 16 }}>
-        <button className={tab === "rules" ? "active" : ""} onClick={() => setTab("rules")}><Wand2 size={15} />  {t("Rules")}</button>
-        <button className={tab === "scripts" ? "active" : ""} onClick={() => setTab("scripts")}><Code size={15} />  {t("Scripts (advanced)")}</button>
+        <button className={tab === "rules" ? "active" : ""} onClick={() => switchTab("rules")}><Wand2 size={15} />  {t("Rules")}</button>
+        <button className={tab === "scripts" ? "active" : ""} onClick={() => switchTab("scripts")}><Code size={15} />  {t("Scripts (advanced)")}</button>
       </div>
       {sieve.loading && !sieve.scripts.length ? <Spinner /> : tab === "rules" ? <RulesEditor /> : <ScriptsEditor />}
     </div>
@@ -68,19 +80,30 @@ function RulesEditor() {
   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 save = async (next: SieveRule[]) => {
+  const save = async (next: SieveRule[]): Promise<boolean> => {
     setSaving(true);
     try {
       await sieve.saveRules(next);
       setLocal(null);
       toast.success(t("Filters saved"));
+      return true;
     } catch (err) {
       toast.error(t("Could not save filters: {error}", { error: (err as Error).message }));
+      return false;
     } finally {
       setSaving(false);
     }
   };
 
+  // Before the early returns below: a hook cannot be skipped, and the branches
+  // they guard have nothing pending anyway.
+  useUnsavedChanges({
+    dirty,
+    message: t("Your filter rules have changes that have not been saved."),
+    save: () => save(list),
+    discard: () => setLocal(null),
+  });
+
   // Before the hand-written branch: a script that arrived in part is not a
   // script someone chose to write themselves, and the way out of it is a reload
   // rather than the "start with rules" button below, which would write over it.
@@ -149,11 +172,16 @@ function RulesEditor() {
           </div>
         </div>
       ))}
-      <div className="row" style={{ marginTop: 12 }}>
+      {/*
+        * Pinned, because with more than a screenful of rules this bar was the
+        * only thing saying there was unsaved work and it sat below the fold.
+        */}
+      <div className="row save-bar">
         <button className="btn" onClick={() => setEditing(newRule())}><Plus size={16} />  {t("New rule")}</button>
         <span className="spacer" />
+        {dirty && <span className="unsaved">{t("Unsaved changes")}</span>}
         {dirty && <button className="btn btn-ghost" onClick={() => setLocal(null)}>{t("Discard changes")}</button>}
-        <button className="btn btn-primary" disabled={!dirty || saving} onClick={() => void save(list)}>{saving ? "Saving…" : "Save filters"}</button>
+        <button className="btn btn-primary" disabled={!dirty || saving} onClick={() => void save(list)}>{saving ? t("Saving…") : t("Save filters")}</button>
       </div>
       {content && (
         <details style={{ marginTop: 20 }}>
@@ -188,23 +216,39 @@ function ScriptsEditor() {
   const [name, setName] = useState("");
   const [busy, setBusy] = useState(false);
   const [validation, setValidation] = useState<string | null>(null);
+  /**
+   * What was in the editor when it opened, so "has this been touched" is a
+   * comparison rather than a flag every edit path has to remember to set.
+   * `null` means the editor is closed and there is nothing to compare.
+   */
+  const [opened, setOpened] = useState<{ name: string; content: string } | null>(null);
+  const dirty = opened !== null && (name !== opened.name || content !== opened.content);
+
+  const start = (scriptName: string, source: string) => {
+    setName(scriptName);
+    setContent(source);
+    setOpened({ name: scriptName, content: source });
+  };
+
+  const close = () => {
+    setSel(null);
+    setName("");
+    setContent("");
+    setOpened(null);
+    setValidation(null);
+  };
 
   const open = async (s: SieveScript | null) => {
     setSel(s);
     setValidation(null);
-    if (s) {
-      setName(s.name);
-      setContent(await sieve.getContent(s.id));
-    } else {
-      setName("");
-      setContent('require ["fileinto"];\n\n');
-    }
+    if (s) start(s.name, await sieve.getContent(s.id));
+    else start("", 'require ["fileinto"];\n\n');
   };
 
-  const save = async (activate: boolean) => {
+  const save = async (activate: boolean): Promise<boolean> => {
     if (!name.trim()) {
       toast.error(t("Script name is required"));
-      return;
+      return false;
     }
     setBusy(true);
     try {
@@ -212,20 +256,39 @@ function ScriptsEditor() {
       setValidation(err);
       if (err) {
         toast.error(t("Script has errors"));
-        return;
+        return false;
       }
       await sieve.saveScript(sel?.id ?? null, name.trim(), content, activate);
       toast.success(t("Script saved"));
-      setSel(null);
+      // All the way closed, back to the list. Clearing only `sel` left the
+      // editor up -- it is shown whenever there is a name or a body -- but with
+      // the name unlocked, so saving a second time created a duplicate script
+      // rather than updating the one just written.
+      close();
+      return true;
     } catch (err) {
       toast.error((err as Error).message);
+      return false;
     } finally {
       setBusy(false);
     }
   };
 
-  if (sel !== null || name !== "" || content !== "") {
-    if (sel !== null || name !== "" || content !== "") {
+  // A hand-written script is the worst thing here to lose, and until now
+  // leaving the page took it without asking.
+  useUnsavedChanges({
+    dirty,
+    message: t("Your Sieve script has changes that have not been saved."),
+    // Whether this script is the active one is not this dialog's question to
+    // reopen, so the save it offers leaves that exactly as it found it.
+    save: () => save(false),
+    discard: close,
+  });
+
+  // One question, asked once: the editor is up exactly while a script is open
+  // in it. Before, this was a pair of identical nested conditions reading name
+  // and body, which is also why saving could not put the editor away.
+  if (opened !== null) {
     return (
       <div>
         <div className="field"><label>{t("Script name")}</label><input className="input" value={name} onChange={(e) => setName(e.target.value)} disabled={Boolean(sel)} /></div>
@@ -234,17 +297,17 @@ function ScriptsEditor() {
           <textarea className="code notranslate" translate="no" value={content} onChange={(e) => setContent(e.target.value)} spellCheck={false} style={{ minHeight: 320 }} />
         </div>
         {validation && <div className="error-box mb-16">{validation}</div>}
-          <div className="row">
-            <button className="btn btn-ghost" onClick={() => { setSel(null); setName(""); setContent(""); }}>{t("Cancel")}</button>
+        <div className="row save-bar">
+          <button className="btn btn-ghost" onClick={close}>{t("Cancel")}</button>
           <button className="btn" disabled={busy} onClick={async () => { setBusy(true); const err = await sieve.validate(content); setValidation(err); setBusy(false); if (!err) toast.success(t("Script is valid")); }}><Play size={14} />  {t("Validate")}</button>
           <span className="spacer" />
+          {dirty && <span className="unsaved">{t("Unsaved changes")}</span>}
           <button className="btn" disabled={busy} onClick={() => void save(false)}>{t("Save")}</button>
           <button className="btn btn-primary" disabled={busy} onClick={() => void save(true)}>{t("Save & activate")}</button>
         </div>
       </div>
     );
   }
-  }
 
   return (
     <div>
@@ -259,7 +322,7 @@ function ScriptsEditor() {
           </div>
         </div>
       ))}
-      <button className="btn" onClick={async () => { const n = await promptDialog({ title: "New script", placeholder: "Script name" }); if (n) { setName(n); setContent('require ["fileinto"];\n\n'); } }}><Plus size={16} />  {t("New script")}</button>
+      <button className="btn" onClick={async () => { const n = await promptDialog({ title: t("New script"), placeholder: t("Script name") }); if (n) start(n, 'require ["fileinto"];\n\n'); }}><Plus size={16} />  {t("New script")}</button>
     </div>
   );
 }

web/src/views/settings/__tests__/filters-unsaved-guard.test.tsx

@@ -0,0 +1,112 @@
+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 { ConfirmHost } from "@/ui/dialog";
+import { useSieve } from "@/store/sieve";
+import { hasUnsavedChanges } from "@/lib/unsavedChanges";
+import { newRule, rulesToSieve } from "@/lib/sieve";
+
+(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+
+/**
+ * Everything here is a click, and a click can answer a dialog. Answering one
+ * resolves a promise that another promise is waiting on -- discard, then
+ * continue, then navigate -- so settle the queue rather than a single tick of
+ * it, or the assertion runs a link in the chain too early.
+ */
+const click = async (el: Element | null | undefined) => {
+  expect(el, "nothing to click").toBeTruthy();
+  await act(async () => {
+    el!.dispatchEvent(new MouseEvent("click", { bubbles: true }));
+  });
+  await act(async () => {
+    await new Promise((r) => setTimeout(r, 0));
+  });
+};
+
+describe("leaving the filter editors with unsaved changes", () => {
+  let host: HTMLDivElement;
+  let root: Root;
+  let live = false;
+  const unmount = () => { if (live) { act(() => root.unmount()); live = false; } };
+  /**
+   * The dialog queue is a module-level store, so a question nobody answered
+   * outlives the test that asked it and is the one the next test finds on
+   * screen. Answer whatever is still up -- by dismissing, which changes
+   * nothing -- while the host that renders it is still mounted.
+   */
+  const drain = async () => {
+    for (let i = 0; i < 5 && document.querySelector(".dialog"); i++) {
+      await click(document.querySelector(".dialog-foot .btn"));
+    }
+  };
+
+  const byText = (sel: string, text: string) => Array.from(document.querySelectorAll(sel)).find((e) => e.textContent?.includes(text));
+  const tab = (label: string) => byText(".view-switch button", label);
+  const dialogChoice = (label: string) => byText(".dialog-choice", label);
+  const onScriptsTab = () => Boolean(document.body.textContent?.includes("manage raw Sieve scripts"));
+  /** The first rule's on/off switch: flipping it is the smallest possible edit. */
+  const firstToggle = () => document.querySelector(".rule-card .switch");
+
+  beforeEach(() => {
+    const rules = ["Newsletters", "From the boss"].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 /><ConfirmHost /></>));
+    live = true;
+  });
+  afterEach(async () => { await drain(); unmount(); host.remove(); });
+
+  it("has nothing to ask about until something is edited", async () => {
+    expect(hasUnsavedChanges()).toBe(false);
+    await click(tab("Scripts"));
+    expect(onScriptsTab()).toBe(true);
+  });
+
+  it("says so on screen as soon as there is unsaved work", async () => {
+    expect(document.querySelector(".save-bar .unsaved")).toBeNull();
+    await click(firstToggle());
+    expect(hasUnsavedChanges()).toBe(true);
+    expect(document.querySelector(".save-bar .unsaved")).not.toBeNull();
+  });
+
+  it("asks before a tab switch throws the edits away", async () => {
+    await click(firstToggle());
+    await click(tab("Scripts"));
+    // Still here, and still asking.
+    expect(onScriptsTab()).toBe(false);
+    expect(dialogChoice("Discard")).toBeTruthy();
+  });
+
+  it("stays put, edits intact, when the question is dismissed", async () => {
+    await click(firstToggle());
+    await click(tab("Scripts"));
+    await click(document.querySelector(".dialog-foot .btn"));
+    expect(document.querySelector(".dialog")).toBeNull();
+    expect(onScriptsTab()).toBe(false);
+    expect(hasUnsavedChanges()).toBe(true);
+    expect(document.querySelector(".save-bar .unsaved")).not.toBeNull();
+  });
+
+  it("goes on to the other tab once the edits are discarded", async () => {
+    await click(firstToggle());
+    await click(tab("Scripts"));
+    await click(dialogChoice("Discard"));
+    expect(onScriptsTab()).toBe(true);
+    expect(hasUnsavedChanges()).toBe(false);
+  });
+
+  it("registers nothing once the editor is gone", async () => {
+    await click(firstToggle());
+    expect(hasUnsavedChanges()).toBe(true);
+    unmount();
+    expect(hasUnsavedChanges()).toBe(false);
+  });
+});