From 3f88868aad92f8b7f40c0090cfcb212594440328 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:20:33 +0200 Subject: [PATCH] fix: keep folder ids, forward copies and flag order in filters saved on the phone The webmail now stores the target folder's id on move and copy actions and writes `fileinto :mailboxid "" ""` when the server lists `mailboxid`, so a rule keeps working after the folder is renamed. It also offers "Keep a copy" on forwards (`redirect :copy`) and sets flags before the move, because `fileinto` stores the flags set at that moment (dadfb08f). The phone did none of this: saving filters on the phone turned every `:mailboxid` target back into a bare path, forwards with a copy into plain redirects, and a rule that stars and moves mail stopped starring it if the move was picked first. Port the generator and parser changes and pass the server's Sieve extensions to the generator. In the rule editor: - folders are picked by id, and saving stores the id next to the path and refreshes the path from it, so older rules pick up the id; - a folder that isn't listed stays selected instead of being replaced; - forwards get the "Keep a copy" switch; - changing an action's type drops the id and the copy flag it no longer uses. --- src/components/filters/FilterRuleModal.tsx | 112 ++++++++++-------- .../fixtures/webmail/folder-targets.sieve | 27 +++++ src/lib/sieve/__tests__/generator.test.ts | 46 +++++++ src/lib/sieve/__tests__/rule-actions.test.ts | 108 +++++++++++++++++ .../sieve/__tests__/webmail-fixtures.test.ts | 8 +- src/lib/sieve/generator.ts | 52 ++++++-- src/lib/sieve/parser.ts | 17 ++- src/lib/sieve/rule-actions.ts | 98 +++++++++++++++ src/lib/sieve/types.ts | 9 ++ src/stores/__tests__/filter-store.test.ts | 16 +++ src/stores/filter-store.ts | 12 +- 11 files changed, 440 insertions(+), 65 deletions(-) create mode 100644 src/lib/sieve/__tests__/fixtures/webmail/folder-targets.sieve create mode 100644 src/lib/sieve/__tests__/rule-actions.test.ts create mode 100644 src/lib/sieve/rule-actions.ts diff --git a/src/components/filters/FilterRuleModal.tsx b/src/components/filters/FilterRuleModal.tsx index 7402234c..acd8fc52 100644 --- a/src/components/filters/FilterRuleModal.tsx +++ b/src/components/filters/FilterRuleModal.tsx @@ -12,7 +12,6 @@ import Input from '../Input'; import Button from '../Button'; import { useLocaleStore } from '../../stores/locale-store'; import { useKeywordsStore } from '../../stores/keywords-store'; -import { buildMailboxTree, type MailboxNode } from '../../lib/mailbox-tree'; import { generateUUID } from '../../lib/uuid'; import { inputStringToValue, @@ -20,6 +19,15 @@ import { isHasAnyCondition, valueToInputString, } from '../../lib/sieve/condition-value'; +import { + ACTIONS_WITH_MAILBOX, + ACTIONS_WITH_VALUE, + buildMailboxTargets, + mailboxIdFor, + selectMailboxTarget, + updateFilterAction, + withMailboxTarget, +} from '../../lib/sieve/rule-actions'; import type { Mailbox } from '../../api/types'; import type { FilterRule, @@ -51,9 +59,6 @@ function seedActions(rule?: FilterRule): FilterAction[] { } const ALL_ACTION_TYPES: FilterActionType[] = ['move', 'copy', 'forward', 'mark_read', 'star', 'add_label', 'discard', 'reject', 'keep', 'stop']; -const ACTIONS_WITH_VALUE = new Set(['move', 'copy', 'forward', 'reject', 'add_label']); -const ACTIONS_WITH_MAILBOX = new Set(['move', 'copy']); - function makeEmptyCondition(): FilterCondition { return { field: 'from', comparator: 'contains', value: '' }; } @@ -69,23 +74,6 @@ interface FilterRuleModalProps { onClose: () => void; } -// Flatten the mailbox tree into pickable options, building the Sieve-canonical -// folder path for each (inbox -> "INBOX", not the localized display name). -function buildMailboxOptions(mailboxes: Mailbox[]): { value: string; label: string }[] { - const tree = buildMailboxTree(mailboxes); - const options: { value: string; label: string }[] = []; - const walk = (nodes: MailboxNode[], parentPath: string) => { - for (const node of nodes) { - const segment = node.role === 'inbox' ? 'INBOX' : node.name; - const fullPath = parentPath ? `${parentPath}/${segment}` : segment; - options.push({ value: fullPath, label: `${' '.repeat(node.depth * 3)}${node.name}` }); - if (node.children.length > 0) walk(node.children, fullPath); - } - }; - walk(tree, ''); - return options; -} - export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: FilterRuleModalProps) { const c = useColors(); const styles = useMemo(() => makeStyles(c), [c]); @@ -112,7 +100,7 @@ export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: F setStopProcessing(rule?.stopProcessing ?? false); }, [visible, rule]); - const mailboxOptions = useMemo(() => buildMailboxOptions(mailboxes), [mailboxes]); + const mailboxTargets = useMemo(() => buildMailboxTargets(mailboxes), [mailboxes]); const fieldOptions = useMemo( () => ALL_FIELDS.map((f) => ({ value: f, label: t(`settings.filters.condition_fields.${f}`, f) })), @@ -165,20 +153,28 @@ export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: F const updateAction = (index: number, updates: Partial) => { setActions((prev) => - prev.map((act, i) => { - if (i !== index) return act; - const updated = { ...act, ...updates }; - if (updates.type && !ACTIONS_WITH_VALUE.has(updates.type)) { - delete updated.value; - } - if (updates.type && ACTIONS_WITH_MAILBOX.has(updates.type) && !updated.value) { - updated.value = mailboxOptions[0]?.value ?? ''; - } - return updated; - }), + prev.map((act, i) => (i === index ? updateFilterAction(act, updates, mailboxTargets) : act)), ); }; + const selectFolder = (index: number, id: string) => { + setActions((prev) => + prev.map((act, i) => (i === index ? selectMailboxTarget(act, id, mailboxTargets) : act)), + ); + }; + + // Folder picker rows for an action. A folder the rule points at that isn't + // listed (deleted, or its account's folders not loaded) stays selectable + // under its path, so saving doesn't silently retarget the rule. + const folderOptions = (action: FilterAction) => { + const options = mailboxTargets.map((target) => ({ value: target.id, label: target.label })); + const selected = mailboxIdFor(action, mailboxTargets); + if (!options.some((o) => o.value === selected) && (selected || action.value)) { + options.unshift({ value: selected, label: action.value || selected }); + } + return options; + }; + const removeAction = (index: number) => { if (actions.length <= 1) return; setActions((prev) => prev.filter((_, i) => i !== index)); @@ -206,7 +202,9 @@ export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: F Alert.alert(t('settings.filters.validation_empty_conditions', 'At least one condition with a value is required')); return; } - const validActions = actions.filter((a) => !ACTIONS_WITH_VALUE.has(a.type) || a.value?.trim()); + const validActions = actions + .map((a) => withMailboxTarget(a, mailboxTargets)) + .filter((a) => !ACTIONS_WITH_VALUE.has(a.type) || a.value?.trim()); if (validActions.length === 0) { Alert.alert(t('settings.filters.validation_empty_actions', 'At least one action is required')); return; @@ -220,7 +218,7 @@ export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: F actions: validActions, stopProcessing, }); - }, [name, conditions, actions, matchType, stopProcessing, rule, onSave, t]); + }, [name, conditions, actions, matchType, stopProcessing, rule, onSave, t, mailboxTargets]); return ( @@ -364,30 +362,42 @@ export function FilterRuleModal({ visible, rule, mailboxes, onSave, onClose }: F {ACTIONS_WITH_MAILBOX.has(action.type) && ( - mailboxOptions.length > 0 ? ( + mailboxTargets.length > 0 ? ( updateAction(index, { value: v })} + onChangeText={(v) => updateAction(index, { value: v, mailboxId: undefined })} placeholder={t('settings.filters.move_to_folder', 'Select folder')} /> ) )} {action.type === 'forward' && ( - updateAction(index, { value: v })} - placeholder={t('settings.filters.forward_placeholder', 'email@example.com')} - keyboardType="email-address" - autoCapitalize="none" - /> + <> + updateAction(index, { value: v })} + placeholder={t('settings.filters.forward_placeholder', 'email@example.com')} + keyboardType="email-address" + autoCapitalize="none" + /> + + + {t('settings.filters.forward_keep_copy', 'Keep a copy')} + + updateAction(index, { keepCopy: v || undefined })} + /> + + )} {action.type === 'reject' && ( @@ -505,6 +515,14 @@ function makeStyles(c: ThemePalette) { }, stopLabel: { ...typography.body, color: c.text, flex: 1 }, + optionRow: { + flexDirection: 'row', + alignItems: 'center', + justifyContent: 'space-between', + gap: spacing.md, + }, + optionLabel: { ...typography.body, color: c.text, flex: 1 }, + footer: { flexDirection: 'row', justifyContent: 'flex-end', diff --git a/src/lib/sieve/__tests__/fixtures/webmail/folder-targets.sieve b/src/lib/sieve/__tests__/fixtures/webmail/folder-targets.sieve new file mode 100644 index 00000000..3cb6df4d --- /dev/null +++ b/src/lib/sieve/__tests__/fixtures/webmail/folder-targets.sieve @@ -0,0 +1,27 @@ +/* @metadata:begin +{"version":1,"rules":[{"id":"invoices","name":"Invoices","enabled":true,"matchType":"all","conditions":[{"field":"subject","comparator":"contains","value":["invoice","Rechnung"]}],"actions":[{"type":"move","value":"Finance/Invoices","mailboxId":"m12"},{"type":"star"},{"type":"add_label","value":"finance"}],"stopProcessing":false},{"id":"backup","name":"Backup","enabled":true,"matchType":"all","conditions":[{"field":"from","comparator":"contains","value":"news@example.com"}],"actions":[{"type":"copy","value":"Archive","mailboxId":"m7"}],"stopProcessing":false},{"id":"legacy","name":"Legacy path","enabled":true,"matchType":"all","conditions":[{"field":"from","comparator":"contains","value":"news@example.com"}],"actions":[{"type":"move","value":"Old/Path"}],"stopProcessing":false},{"id":"fwd","name":"Forward","enabled":true,"matchType":"all","conditions":[{"field":"from","comparator":"contains","value":"news@example.com"}],"actions":[{"type":"forward","value":"me@elsewhere.example","keepCopy":true}],"stopProcessing":false}]} +@metadata:end */ + +require ["copy", "fileinto", "imap4flags", "mailbox", "mailboxid"]; + +# Rule: Invoices +if header :contains "Subject" ["invoice", "Rechnung"] { + addflag "\\Flagged"; + addflag "$label:finance"; + fileinto :mailboxid "m12" "Finance/Invoices"; +} + +# Rule: Backup +if header :contains "From" "news@example.com" { + fileinto :copy :mailboxid "m7" "Archive"; +} + +# Rule: Legacy path +if header :contains "From" "news@example.com" { + fileinto "Old/Path"; +} + +# Rule: Forward +if header :contains "From" "news@example.com" { + redirect :copy "me@elsewhere.example"; +} diff --git a/src/lib/sieve/__tests__/generator.test.ts b/src/lib/sieve/__tests__/generator.test.ts index 9bbb01c6..191318e3 100644 --- a/src/lib/sieve/__tests__/generator.test.ts +++ b/src/lib/sieve/__tests__/generator.test.ts @@ -260,6 +260,52 @@ describe('generateScript', () => { }); }); + describe('action order and targets', () => { + it('sets flags before the move so the moved message keeps them', () => { + const script = generateScript([makeRule({ + actions: [{ type: 'move', value: 'Archive' }, { type: 'star' }, { type: 'mark_read' }], + })]); + const flagged = script.indexOf('addflag "\\\\Flagged";'); + const seen = script.indexOf('addflag "\\\\Seen";'); + const move = script.indexOf('fileinto "Archive";'); + expect(flagged).toBeGreaterThan(-1); + expect(flagged).toBeLessThan(move); + expect(seen).toBeLessThan(move); + }); + + it('targets the folder id when the server supports mailboxid', () => { + const rules = [makeRule({ actions: [{ type: 'move', value: 'Work/Old', mailboxId: 'm42' }] })]; + const script = generateScript(rules, undefined, { extensions: ['fileinto', 'mailbox', 'mailboxid'] }); + expect(script).toContain('fileinto :mailboxid "m42" "Work/Old";'); + expect(script).toContain('require ["fileinto", "mailbox", "mailboxid"];'); + }); + + it('falls back to the path without mailboxid support', () => { + const rules = [makeRule({ actions: [{ type: 'copy', value: 'Work', mailboxId: 'm42' }] })]; + const script = generateScript(rules, undefined, { extensions: ['fileinto', 'copy'] }); + expect(script).toContain('fileinto :copy "Work";'); + expect(script).not.toContain('mailboxid'); + expect(parseScript(script).rules[0].actions[0]).toEqual({ type: 'copy', value: 'Work', mailboxId: 'm42' }); + }); + + it('keeps a copy of forwarded mail when asked', () => { + const script = generateScript([makeRule({ actions: [{ type: 'forward', value: 'a@b.c', keepCopy: true }] })]); + expect(script).toContain('redirect :copy "a@b.c";'); + expect(script).toContain('require ["copy"];'); + }); + + it('parses the new action forms from external scripts', () => { + const parsed = parseScript( + 'require ["fileinto", "copy", "mailbox", "mailboxid"];\n' + + 'if header :contains "From" "x" { fileinto :copy :mailboxid "m1" "A"; redirect :copy "a@b.c"; }', + ); + expect(parsed.rules[0].actions).toEqual([ + { type: 'copy', value: 'A', mailboxId: 'm1' }, + { type: 'forward', value: 'a@b.c', keepCopy: true }, + ]); + }); + }); + describe('vacation include', () => { it('includes the server vacation script before the rules', () => { const script = generateScript([makeRule()], undefined, { includeVacation: true }); diff --git a/src/lib/sieve/__tests__/rule-actions.test.ts b/src/lib/sieve/__tests__/rule-actions.test.ts new file mode 100644 index 00000000..9bbecf45 --- /dev/null +++ b/src/lib/sieve/__tests__/rule-actions.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect } from 'vitest'; +import type { Mailbox } from '../../../api/types'; +import { + buildMailboxTargets, + mailboxIdFor, + selectMailboxTarget, + updateFilterAction, + withMailboxTarget, +} from '../rule-actions'; + +const RIGHTS = { + mayReadItems: true, mayAddItems: true, mayRemoveItems: true, maySetSeen: true, maySetKeywords: true, + mayCreateChild: true, mayRename: true, mayDelete: true, maySubmit: true, +}; + +function mailbox(id: string, name: string, extra: Partial = {}): Mailbox { + return { + id, name, parentId: null, role: null, sortOrder: 0, + totalEmails: 0, unreadEmails: 0, totalThreads: 0, unreadThreads: 0, + myRights: RIGHTS, ...extra, + }; +} + +const MAILBOXES = [ + mailbox('m1', 'Posteingang', { role: 'inbox' }), + mailbox('m2', 'Finance'), + mailbox('m3', 'Invoices 2026', { parentId: 'm2' }), +]; +const targets = buildMailboxTargets(MAILBOXES); + +describe('buildMailboxTargets', () => { + it('lists folders with their JMAP id and Sieve path', () => { + expect(targets.map(({ id, path }) => ({ id, path }))).toEqual([ + { id: 'm1', path: 'INBOX' }, + { id: 'm2', path: 'Finance' }, + { id: 'm3', path: 'Finance/Invoices 2026' }, + ]); + }); + + it('uses the raw JMAP id of a shared account\'s folders', () => { + const shared = buildMailboxTargets([ + mailbox('team:x1', 'Support', { originalId: 'x1', accountId: 'team' }), + ]); + expect(shared[0]).toMatchObject({ id: 'x1', path: 'Support' }); + }); +}); + +describe('folder targets of move and copy actions', () => { + it('resolves rules saved before folder ids were stored by their path', () => { + expect(mailboxIdFor({ type: 'move', value: 'Finance/Invoices 2026' }, targets)).toBe('m3'); + expect(mailboxIdFor({ type: 'move', value: 'Gone' }, targets)).toBe(''); + expect(mailboxIdFor({ type: 'move', value: 'Finance', mailboxId: 'm9' }, targets)).toBe('m9'); + }); + + it('stores the id on save and refreshes the path of a renamed folder', () => { + expect(withMailboxTarget({ type: 'move', value: 'Finance' }, targets)) + .toEqual({ type: 'move', value: 'Finance', mailboxId: 'm2' }); + expect(withMailboxTarget({ type: 'copy', value: 'Old name', mailboxId: 'm3' }, targets)) + .toEqual({ type: 'copy', value: 'Finance/Invoices 2026', mailboxId: 'm3' }); + }); + + it('keeps a target it cannot find as it is', () => { + const unknown = { type: 'move' as const, value: 'Elsewhere', mailboxId: 'm99' }; + expect(withMailboxTarget(unknown, targets)).toEqual(unknown); + expect(withMailboxTarget({ type: 'move', value: 'Nowhere' }, targets)).toEqual({ type: 'move', value: 'Nowhere' }); + expect(withMailboxTarget({ type: 'forward', value: 'a@b.c' }, targets)).toEqual({ type: 'forward', value: 'a@b.c' }); + }); + + it('picks a folder by id and keeps the path in step', () => { + const moved = selectMailboxTarget({ type: 'move', value: 'Finance', mailboxId: 'm2' }, 'm3', targets); + expect(moved).toEqual({ type: 'move', value: 'Finance/Invoices 2026', mailboxId: 'm3' }); + }); + + it('leaves an unlisted folder alone when it is picked again', () => { + const unknown = { type: 'move' as const, value: 'Elsewhere', mailboxId: 'm99' }; + expect(selectMailboxTarget(unknown, 'm99', targets)).toBe(unknown); + }); +}); + +describe('updateFilterAction', () => { + it('drops the folder id and the copy flag when the type changes', () => { + expect(updateFilterAction({ type: 'move', value: 'Finance', mailboxId: 'm2' }, { type: 'star' }, targets)) + .toEqual({ type: 'star' }); + expect(updateFilterAction({ type: 'forward', value: 'a@b.c', keepCopy: true }, { type: 'reject' }, targets)) + .toEqual({ type: 'reject', value: 'a@b.c' }); + }); + + it('keeps the folder when switching between move and copy', () => { + expect(updateFilterAction({ type: 'move', value: 'Finance', mailboxId: 'm2' }, { type: 'copy' }, targets)) + .toEqual({ type: 'copy', value: 'Finance', mailboxId: 'm2' }); + }); + + it('starts a new move on the first folder', () => { + expect(updateFilterAction({ type: 'star' }, { type: 'move' }, targets)) + .toEqual({ type: 'move', value: 'INBOX', mailboxId: 'm1' }); + }); + + it('drops the folder id when the path is typed by hand', () => { + expect(updateFilterAction({ type: 'move', value: 'Finance', mailboxId: 'm2' }, { value: 'Typed', mailboxId: undefined }, [])) + .toEqual({ type: 'move', value: 'Typed' }); + }); + + it('toggles keeping a copy of forwarded mail', () => { + const on = updateFilterAction({ type: 'forward', value: 'a@b.c' }, { keepCopy: true }, targets); + expect(on).toEqual({ type: 'forward', value: 'a@b.c', keepCopy: true }); + expect(updateFilterAction(on, { keepCopy: undefined }, targets)).toEqual({ type: 'forward', value: 'a@b.c' }); + }); +}); diff --git a/src/lib/sieve/__tests__/webmail-fixtures.test.ts b/src/lib/sieve/__tests__/webmail-fixtures.test.ts index fb01d165..730506d2 100644 --- a/src/lib/sieve/__tests__/webmail-fixtures.test.ts +++ b/src/lib/sieve/__tests__/webmail-fixtures.test.ts @@ -6,7 +6,7 @@ // // The fixtures in fixtures/webmail/ were written by the webmail's own // generator (jmap-webmail lib/sieve/generator.ts at e33ab899), run on the -// rules in each file's metadata. +// rules in each file's metadata for a server listing `extensions`. import { describe, it, expect } from 'vitest'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; @@ -15,10 +15,13 @@ import { parseScript } from '../parser'; interface WebmailFixture { file: string; + /** The server's sieveExtensions the webmail generated the script for. */ + extensions?: string[]; } const FIXTURES: WebmailFixture[] = [ { file: 'vacation-include.sieve' }, + { file: 'folder-targets.sieve', extensions: ['fileinto', 'copy', 'imap4flags', 'mailbox', 'mailboxid'] }, ]; function readFixture(file: string): string { @@ -32,7 +35,7 @@ function metadataRules(script: string): unknown[] { } describe('scripts written by the webmail', () => { - for (const { file } of FIXTURES) { + for (const { file, extensions } of FIXTURES) { describe(file, () => { const script = readFixture(file); const parsed = parseScript(script); @@ -47,6 +50,7 @@ describe('scripts written by the webmail', () => { const regenerated = generateScript(parsed.rules, parsed.vacation, { externalRequires: parsed.externalRequires, includeVacation: parsed.includeVacation, + extensions, }); expect(regenerated).toBe(script); }); diff --git a/src/lib/sieve/generator.ts b/src/lib/sieve/generator.ts index b22c910e..17625951 100644 --- a/src/lib/sieve/generator.ts +++ b/src/lib/sieve/generator.ts @@ -109,15 +109,31 @@ function generateCondition(condition: FilterCondition): string { } } -function generateActions(actions: FilterAction[]): string[] { - return actions.map(action => { +const FLAG_ACTIONS = new Set(['mark_read', 'star', 'add_label']); + +function fileintoTarget(action: FilterAction, useMailboxId: boolean): string { + const path = `"${escapeString(action.value || '')}"`; + // RFC 9042: the path is the fallback when the id no longer exists. + return useMailboxId && action.mailboxId + ? `:mailboxid "${escapeString(action.mailboxId)}" ${path}` + : path; +} + +function generateActions(actions: FilterAction[], useMailboxId: boolean): string[] { + // fileinto (and the implicit keep) store the flags set at that moment, so a + // flag action placed after a move in the UI would be lost. Set flags first. + const ordered = [ + ...actions.filter(a => FLAG_ACTIONS.has(a.type)), + ...actions.filter(a => !FLAG_ACTIONS.has(a.type)), + ]; + return ordered.map(action => { switch (action.type) { case 'move': - return `fileinto "${escapeString(action.value || '')}";`; + return `fileinto ${fileintoTarget(action, useMailboxId)};`; case 'copy': - return `fileinto :copy "${escapeString(action.value || '')}";`; + return `fileinto :copy ${fileintoTarget(action, useMailboxId)};`; case 'forward': - return `redirect "${escapeString(action.value || '')}";`; + return `redirect ${action.keepCopy ? ':copy ' : ''}"${escapeString(action.value || '')}";`; case 'mark_read': return 'addflag "\\\\Seen";'; case 'star': @@ -140,7 +156,7 @@ function generateActions(actions: FilterAction[]): string[] { }); } -function computeRequires(rules: FilterRule[], vacation?: VacationSieveConfig): string[] { +function computeRequires(rules: FilterRule[], vacation: VacationSieveConfig | undefined, useMailboxId: boolean): string[] { const extensions = new Set(); const enabledRules = rules.filter(r => r.enabled); @@ -156,12 +172,20 @@ function computeRequires(rules: FilterRule[], vacation?: VacationSieveConfig): s for (const action of rule.actions) { switch (action.type) { case 'move': + case 'copy': + extensions.add('fileinto'); + if (action.type === 'copy') extensions.add('copy'); + if (useMailboxId && action.mailboxId) { + // Stalwart rejects :mailboxid unless "mailbox" is declared too. + extensions.add('mailbox'); + extensions.add('mailboxid'); + } + break; case 'keep': extensions.add('fileinto'); break; - case 'copy': - extensions.add('fileinto'); - extensions.add('copy'); + case 'forward': + if (action.keepCopy) extensions.add('copy'); break; case 'mark_read': case 'star': @@ -203,6 +227,11 @@ export interface GenerateOptions { * while the filters stay active. */ includeVacation?: boolean; + /** + * The server's `sieveExtensions`. Folder moves use `:mailboxid` when it + * lists "mailboxid"; without it they target the folder path only. + */ + extensions?: string[]; } /** Name of the script a server builds for VacationResponse (RFC 9661). */ @@ -239,7 +268,8 @@ export function generateScript( lines.push('@metadata:end */'); lines.push(''); - const bulwarkRequires = computeRequires(bulwarkRules, vacation); + const useMailboxId = options.extensions?.includes('mailboxid') ?? false; + const bulwarkRequires = computeRequires(bulwarkRules, vacation, useMailboxId); if (options.includeVacation) bulwarkRequires.push('include'); const externalRequires = options.externalRequires ?? []; const allRequires = [...new Set([...bulwarkRequires, ...externalRequires])].sort(); @@ -290,7 +320,7 @@ export function generateScript( conditionStr = `${wrapper}(${conditions.join(', ')})`; } - const actionLines = generateActions(rule.actions); + const actionLines = generateActions(rule.actions, useMailboxId); if (rule.stopProcessing) { const lastAction = rule.actions[rule.actions.length - 1]; diff --git a/src/lib/sieve/parser.ts b/src/lib/sieve/parser.ts index c239afe2..57445a13 100644 --- a/src/lib/sieve/parser.ts +++ b/src/lib/sieve/parser.ts @@ -529,8 +529,21 @@ function parseAction(raw: string): FilterAction | null { m = /^fileinto\s+"((?:[^"\\]|\\.)*)"$/.exec(s); if (m) return { type: 'move', value: unescapeSieveString(m[1]) }; - m = /^redirect\s+"((?:[^"\\]|\\.)*)"$/.exec(s); - if (m) return { type: 'forward', value: unescapeSieveString(m[1]) }; + m = /^fileinto\s+(:copy\s+)?:mailboxid\s+"((?:[^"\\]|\\.)*)"\s+"((?:[^"\\]|\\.)*)"$/.exec(s); + if (m) { + return { + type: m[1] ? 'copy' : 'move', + value: unescapeSieveString(m[3]), + mailboxId: unescapeSieveString(m[2]), + }; + } + + m = /^redirect\s+(:copy\s+)?"((?:[^"\\]|\\.)*)"$/.exec(s); + if (m) { + const action: FilterAction = { type: 'forward', value: unescapeSieveString(m[2]) }; + if (m[1]) action.keepCopy = true; + return action; + } m = /^addflag\s+"((?:[^"\\]|\\.)*)"$/.exec(s); if (m) { diff --git a/src/lib/sieve/rule-actions.ts b/src/lib/sieve/rule-actions.ts new file mode 100644 index 00000000..2215c8c5 --- /dev/null +++ b/src/lib/sieve/rule-actions.ts @@ -0,0 +1,98 @@ +import type { Mailbox } from '../../api/types'; +import { buildMailboxTree, type MailboxNode } from '../mailbox-tree'; +import type { FilterAction, FilterActionType } from './types'; + +// Editing helpers for the rule editor's action rows, ported from the +// webmail's components/filters/filter-rule-modal.tsx (dadfb08f). Kept out of +// the component so the folder-id bookkeeping can be tested on its own. + +export const ACTIONS_WITH_VALUE = new Set(['move', 'copy', 'forward', 'reject', 'add_label']); +export const ACTIONS_WITH_MAILBOX = new Set(['move', 'copy']); + +export interface MailboxTarget { + /** JMAP id of the folder, what `fileinto :mailboxid` targets. */ + id: string; + /** Sieve folder path, the fallback when the id no longer exists. */ + path: string; + label: string; +} + +/** + * Flatten the mailbox tree into pickable targets, building the Sieve-canonical + * folder path for each (inbox -> "INBOX", not the localized display name). + * A shared account's folders carry their JMAP id in `originalId`. + */ +export function buildMailboxTargets(mailboxes: Mailbox[]): MailboxTarget[] { + const targets: MailboxTarget[] = []; + const walk = (nodes: MailboxNode[], parentPath: string) => { + for (const node of nodes) { + const segment = node.role === 'inbox' ? 'INBOX' : node.name; + const path = parentPath ? `${parentPath}/${segment}` : segment; + targets.push({ + id: node.originalId ?? node.id, + path, + label: `${' '.repeat(node.depth * 3)}${node.name}`, + }); + if (node.children.length > 0) walk(node.children, path); + } + }; + walk(buildMailboxTree(mailboxes), ''); + return targets; +} + +/** + * Target folder id of a move/copy action. Rules saved before folder ids were + * stored only carry the path, so resolve it; '' when nothing matches. + */ +export function mailboxIdFor(action: FilterAction, targets: MailboxTarget[]): string { + if (action.mailboxId) return action.mailboxId; + return targets.find((t) => t.path === action.value)?.id ?? ''; +} + +/** + * Apply an edit to an action row. Fields the (new) type doesn't use are + * dropped, and a row switched to move/copy starts on the first folder. + */ +export function updateFilterAction( + action: FilterAction, + updates: Partial, + targets: MailboxTarget[], +): FilterAction { + const updated: FilterAction = { ...action, ...updates }; + if (updates.type && !ACTIONS_WITH_VALUE.has(updates.type)) delete updated.value; + if (updates.type && !ACTIONS_WITH_MAILBOX.has(updates.type)) delete updated.mailboxId; + if (updates.type && updates.type !== 'forward') delete updated.keepCopy; + if (!updated.mailboxId) delete updated.mailboxId; + if (!updated.keepCopy) delete updated.keepCopy; + if (updates.type && ACTIONS_WITH_MAILBOX.has(updates.type) && !updated.value) { + const first = targets[0]; + updated.value = first?.path ?? ''; + if (first) updated.mailboxId = first.id; + } + return updated; +} + +/** Point a move/copy row at the folder with this id (from the folder picker). */ +export function selectMailboxTarget( + action: FilterAction, + id: string, + targets: MailboxTarget[], +): FilterAction { + // Re-picking a folder that isn't listed (deleted, or not loaded) keeps it. + if (id === mailboxIdFor(action, targets)) return action; + const path = targets.find((t) => t.id === id)?.path ?? ''; + return updateFilterAction(action, { mailboxId: id || undefined, value: path }, targets); +} + +/** + * The action as saved: a move/copy stores the folder id next to the path, + * and the path is refreshed from the id, so a renamed folder keeps + * receiving the rule's mail. + */ +export function withMailboxTarget(action: FilterAction, targets: MailboxTarget[]): FilterAction { + if (!ACTIONS_WITH_MAILBOX.has(action.type)) return action; + const mailboxId = mailboxIdFor(action, targets); + if (!mailboxId) return action; + const path = targets.find((t) => t.id === mailboxId)?.path; + return { ...action, mailboxId, value: path ?? action.value }; +} diff --git a/src/lib/sieve/types.ts b/src/lib/sieve/types.ts index f96b7f47..1beaa788 100644 --- a/src/lib/sieve/types.ts +++ b/src/lib/sieve/types.ts @@ -16,6 +16,8 @@ export interface SieveCapabilities { sieveExtensions: string[]; notificationMethods: string[]; externalLists: string[]; + /** RFC 9661: redirects run per message; extra ones are skipped silently. */ + maxNumberRedirects?: number | null; } export type FilterConditionField = @@ -58,6 +60,13 @@ export interface FilterCondition { export interface FilterAction { type: FilterActionType; value?: string; + /** + * move/copy: JMAP id of the target folder. `value` keeps the path as the + * fallback; the id keeps the rule working after the folder is renamed. + */ + mailboxId?: string; + /** forward: also keep the message (`redirect :copy`). */ + keepCopy?: boolean; } export type FilterOrigin = 'bulwark' | 'external' | 'opaque'; diff --git a/src/stores/__tests__/filter-store.test.ts b/src/stores/__tests__/filter-store.test.ts index f809fa23..98fff1c0 100644 --- a/src/stores/__tests__/filter-store.test.ts +++ b/src/stores/__tests__/filter-store.test.ts @@ -214,3 +214,19 @@ describe('filter-store and the server vacation script', () => { }); }); }); + +describe('saving scripts the webmail wrote', () => { + it('keeps folder ids, forward copies and the flag order', async () => { + const script = webmailFixture('folder-targets.sieve'); + api.getSieveCapabilities.mockReturnValue({ + ...WITH_INCLUDE, + sieveExtensions: ['fileinto', 'copy', 'imap4flags', 'mailbox', 'mailboxid'], + }); + api.getSieveScripts.mockResolvedValue([{ id: 's1', name: 'filters', blobId: 'b1', isActive: true }]); + api.getSieveScriptContent.mockResolvedValue(script); + + await useFilterStore.getState().selectAccount(null); + await useFilterStore.getState().saveFilters(); + expect(api.updateSieveScript).toHaveBeenCalledWith('s1', script, true, 'own'); + }); +}); diff --git a/src/stores/filter-store.ts b/src/stores/filter-store.ts index 74219a8b..0418a577 100644 --- a/src/stores/filter-store.ts +++ b/src/stores/filter-store.ts @@ -152,13 +152,17 @@ export const useFilterStore = create()((set, get) => ({ try { const { isOpaque, rawScript, rules, activeScriptId, vacationSettings, externalRequires, includeVacation, - selectedAccountId, + selectedAccountId, sieveCapabilities, } = get(); const accountId = selectedAccountId ?? undefined; const content = isOpaque ? rawScript - : generateScript(rules, vacationSettings || undefined, { externalRequires, includeVacation }); + : generateScript(rules, vacationSettings || undefined, { + externalRequires, + includeVacation, + extensions: sieveCapabilities?.sieveExtensions, + }); if (activeScriptId) { await updateSieveScript(activeScriptId, content, true, accountId); @@ -292,7 +296,8 @@ export async function isVacationIncludedInFilters(accountId?: string): Promise { const sieveAccountId = accountId ?? getSieveAccountId(); - if (enabled && !supportsInclude(getSieveCapabilities(sieveAccountId))) return; + const capabilities = getSieveCapabilities(sieveAccountId); + if (enabled && !supportsInclude(capabilities)) return; const { vacationScript, target, parsed } = await loadManagedScript(sieveAccountId); if (!target || !parsed) return; @@ -307,6 +312,7 @@ export async function syncVacationWithFilters(enabled: boolean, accountId?: stri const content = generateScript(parsed.rules, parsed.vacation, { externalRequires: parsed.externalRequires, includeVacation: enabled, + extensions: capabilities?.sieveExtensions, }); await updateSieveScript(target.id, content, enabled || target.isActive, sieveAccountId);