diff --git a/web/src/store/__tests__/sieve-overwrite.test.ts b/web/src/store/__tests__/sieve-overwrite.test.ts new file mode 100644 index 0000000..12917e9 --- /dev/null +++ b/web/src/store/__tests__/sieve-overwrite.test.ts @@ -0,0 +1,85 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { useSieve } from "@/store/sieve"; +import { newRule, rulesToSieve } from "@/lib/sieve"; +import type { SieveScript } from "@/jmap/types"; + +/** + * Issue #76: adding a filter from a message reported success, and the script + * on the server never held more than two rules. + * + * The chain was three links long, and each looked reasonable alone: + * + * 1. `load()` recorded a *failed* blob fetch as `contents[id] = ""`. + * 2. `sieveToRules("")` returns `[]` — "this script has no rules", which is + * indistinguishable from "we could not read this script". + * 3. Saving writes the whole script from that baseline, so every existing + * rule was deleted, and the UI reported success because the write worked. + * + * The fix is to keep "unknown" and "empty" apart at every step. These pin that: + * an unreadable script must never present as an empty one. + */ + +const SCRIPT: SieveScript = { id: "s1", name: "ihasmail", isActive: true, blobId: "b1" } as SieveScript; +const threeRules = [newRule({ name: "One" }), newRule({ name: "Two" }), newRule({ name: "Three" })]; + +beforeEach(() => { + useSieve.setState({ accountId: "a1", scripts: [SCRIPT], contents: {}, loading: false, error: null }); +}); + +describe("a script whose content could not be read", () => { + it("reports its rules as unknown, not as none", () => { + // contents is empty: the fetch failed, or has not happened yet. + const { rules, loaded } = useSieve.getState().rules(); + expect(rules).toBeNull(); + expect(loaded).toBe(false); + }); + + it("refuses to save rather than overwriting what it cannot see", async () => { + await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/could not be read/i); + }); + + it("says so in terms that point at the fix", async () => { + // "Reload and try again" is recoverable advice; a generic failure is not. + await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/reload/i); + }); +}); + +describe("a script that is genuinely empty", () => { + it("is distinguishable from one that could not be read", () => { + useSieve.setState({ contents: { s1: "" } }); + const { rules, loaded } = useSieve.getState().rules(); + expect(loaded).toBe(true); + expect(rules).toEqual([]); + }); +}); + +describe("a script that was read", () => { + it("hands back every rule in it", () => { + useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } }); + const { rules, loaded } = useSieve.getState().rules(); + expect(loaded).toBe(true); + expect(rules).toHaveLength(3); + expect(rules?.map((r) => r.name)).toEqual(["One", "Two", "Three"]); + }); + + it("does not lose rules across a save-shaped round trip", () => { + // The regression in one line: N rules in, N + 1 out after adding one. + useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } }); + const before = useSieve.getState().rules().rules!; + const after = [...before, newRule({ name: "Four" })]; + useSieve.setState({ contents: { s1: rulesToSieve(after) } }); + expect(useSieve.getState().rules().rules).toHaveLength(4); + }); +}); + +describe("reloading", () => { + it("does not discard content it already holds when a refetch yields nothing", () => { + // saveScript caches what it just wrote, then reloads. A reload whose fetch + // failed used to replace the whole map and wipe that. + useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } }); + const kept = useSieve.getState().contents.s1; + useSieve.setState((st) => ({ contents: { ...st.contents } })); // merge, not replace + expect(useSieve.getState().contents.s1).toBe(kept); + expect(useSieve.getState().rules().rules).toHaveLength(3); + }); +}); diff --git a/web/src/store/sieve.ts b/web/src/store/sieve.ts index b5b6631..7a2b5f3 100644 --- a/web/src/store/sieve.ts +++ b/web/src/store/sieve.ts @@ -18,7 +18,8 @@ interface SieveState { load(): Promise; getContent(id: Id): Promise; /** Rules derived from the "ihasmail" script (null = the active script is hand-written). */ - rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string }; + /** `loaded` distinguishes "this script is hand-written" from "we could not read it". */ + rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string; loaded: boolean }; saveRules(rules: SieveRule[]): Promise; saveScript(id: Id | null, name: string, content: string, activate: boolean): Promise; activate(id: Id | null): Promise; @@ -49,18 +50,29 @@ export const useSieve = create((set, get) => ({ try { const res = await client.call>("SieveScript/get", { accountId, ids: null }); set({ scripts: res.list, loading: false, error: null }); - // Preload contents - const contents: Record = {}; + // Preload contents. + // + // A fetch that fails must not be recorded as "". An empty script parses + // to an empty rule list, which reads as "this script has no rules" and is + // indistinguishable from "we could not read this script" -- and the next + // save then writes the whole script out from that empty baseline, + // destroying every rule in it. That is issue #76. + // + // Leaving the key absent instead means `rules()` reports the content as + // unknown, and `saveRules` refuses rather than guessing. + const fetched: Record = {}; await Promise.all( res.list.map(async (s) => { try { - contents[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve"); + fetched[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve"); } catch { - contents[s.id] = ""; + /* leave absent: unknown, not empty */ } }), ); - set({ contents }); + // Merged, not replaced: saveScript caches the content it just wrote, and + // a reload whose fetch failed must not throw that away. + set((st) => ({ contents: { ...st.contents, ...fetched } })); } catch (err) { set({ loading: false, error: (err as Error).message }); } @@ -79,12 +91,24 @@ export const useSieve = create((set, get) => ({ rules() { const { scripts, contents } = get(); const script = scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? scripts.find((s) => s.isActive) ?? null; - const content = script ? (contents[script.id] ?? "") : ""; - return { script, rules: script ? sieveToRules(content) : [], content }; + if (!script) return { script: null, rules: [], content: "", loaded: true }; + const content = contents[script.id]; + // Not loaded, or the fetch failed. `null` means "cannot say", which every + // caller already treats as "do not edit this script" -- as opposed to `[]`, + // which means "this script genuinely has no rules" and invites a save that + // would overwrite whatever is really in it. + if (content === undefined) return { script, rules: null, content: "", loaded: false }; + return { script, rules: sieveToRules(content), content, loaded: true }; }, async saveRules(rules) { const existing = get().scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? null; + // The last line of defence. Writing rules replaces the whole script, so + // doing it from a baseline we never managed to read deletes whatever was + // there. Refusing is recoverable; overwriting is not. + if (existing && get().contents[existing.id] === undefined) { + throw new Error("Your filter script could not be read, so saving would overwrite it. Reload and try again."); + } await get().saveScript(existing?.id ?? null, IHASMAIL_SCRIPT, rulesToSieve(rules), true); }, diff --git a/web/src/views/mail/FilterFromMessage.tsx b/web/src/views/mail/FilterFromMessage.tsx index 4651379..e293470 100644 --- a/web/src/views/mail/FilterFromMessage.tsx +++ b/web/src/views/mail/FilterFromMessage.tsx @@ -33,11 +33,21 @@ export function FilterFromMessageDialog({ email, mailboxId, onClose }: { email: } if (!ready) return ; - const { rules } = sieve.rules(); + const { rules, loaded } = sieve.rules(); if (rules === null) { return ( Close}> -

Your active Sieve script was written by hand, so rules can't be added automatically. Open Settings → Filters & rules to edit the script or switch to managed rules.

+ {/* + Two different situations, and telling them apart matters: one is + permanent and one is a reload away. Saying "written by hand" when the + script merely failed to fetch sends someone looking for a problem + they do not have. + */} + {loaded ? ( +

Your active Sieve script was written by hand, so rules can't be added automatically. Open Settings → Filters & rules to edit the script or switch to managed rules.

+ ) : ( +

Your filter script couldn't be read just now, so adding a rule would risk overwriting it. Reload the page and try again.

+ )}
); }