From d3e173c9c16b802478aa9753400b458003fa126d Mon Sep 17 00:00:00 2001 From: John Coffey Date: Tue, 1 Sep 2026 07:05:17 -0700 Subject: [PATCH] Ask before the filter editors lose your changes Both editors on the Filters & rules page kept their edits in component state, so every way out of the page threw them away without a word: a settings link, the app rail, even the Rules/Scripts switch. The only sign there had been anything to lose was a Save button that a screenful of rules had already pushed below the fold. Editors now register what they have pending, and every in-app navigation asks first -- offering to save, rather than making "leave without saving" the easy answer and saving the one you have to back out and find. Wouter routes links, redirects and navigate() through one place, so the guard holds for the app rail and the settings nav without either knowing an editor exists. The Rules/Scripts switch asks for itself, since it never reaches the router. Reload and tab close get the browser's own prompt. The save bar is pinned to the foot of the pane, so "Unsaved changes" is on screen whether or not the rules fit in the window. Two things fixed on the way past, both in the raw script editor: saving cleared only the selection, which left the editor open with the name unlocked so a second save created a duplicate script instead of updating the one just written; and the pair of identical nested conditions that decided whether the editor was open at all is now the one question it was asking twice. Fixes #175 --- web/src/App.tsx | 28 +++- web/src/lib/unsavedChanges.ts | 107 ++++++++++++++ web/src/styles/app.css | 16 ++- web/src/views/settings/FiltersSettings.tsx | 135 +++++++++++++----- .../__tests__/filters-unsaved-guard.test.tsx | 112 +++++++++++++++ 5 files changed, 357 insertions(+), 41 deletions(-) create mode 100644 web/src/lib/unsavedChanges.ts create mode 100644 web/src/views/settings/__tests__/filters-unsaved-guard.test.tsx diff --git a/web/src/App.tsx b/web/src/App.tsx index eae8f58..43d6076 100644 --- a/web/src/App.tsx +++ b/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. + */ + { + if (!hasUnsavedChanges()) { + navigate(to, options); + return; + } + void confirmLeaveUnsaved().then((ok) => { + if (ok) navigate(to, options); + }); + }} + > {status === "anonymous" ? : } - + ); } diff --git a/web/src/lib/unsavedChanges.ts b/web/src/lib/unsavedChanges.ts new file mode 100644 index 0000000..0ac475a --- /dev/null +++ b/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; + /** 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(); + +/** 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 { + /* + * 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; +} diff --git a/web/src/styles/app.css b/web/src/styles/app.css index e7f5fee..14a3406 100644 --- a/web/src/styles/app.css +++ b/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; } diff --git a/web/src/views/settings/FiltersSettings.tsx b/web/src/views/settings/FiltersSettings.tsx index 5b77317..f510e3c 100644 --- a/web/src/views/settings/FiltersSettings.tsx +++ b/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() {

{t("Filters & rules")}

{t("Sort incoming mail automatically. Rules run on the server (Sieve), so they work for every client you use.")}

- - + +
{sieve.loading && !sieve.scripts.length ? : tab === "rules" ? : } @@ -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 => { 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() { ))} -
+ {/* + * 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. + */} +
+ {dirty && {t("Unsaved changes")}} {dirty && } - +
{content && (
@@ -188,23 +216,39 @@ function ScriptsEditor() { const [name, setName] = useState(""); const [busy, setBusy] = useState(false); const [validation, setValidation] = useState(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 => { if (!name.trim()) { toast.error(t("Script name is required")); - return; + return false; } setBusy(true); try { @@ -212,38 +256,57 @@ 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 !== "") { - return ( -
-
setName(e.target.value)} disabled={Boolean(sel)} />
-
- -