From 98dc13fe9c31985f1e61d5acd2132b391e3a9552 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 04:50:15 +0100 Subject: [PATCH] fix: stop a series edit or delete after an account switch instead of finishing it in the next account --- src/components/calendar/EventModal.tsx | 7 ++ .../__tests__/recurrence-scope-edit.test.ts | 77 +++++++++++++++++++ src/lib/recurrence-scope-edit.ts | 41 +++++++++- src/screens/CalendarScreen.tsx | 11 ++- .../calendar-account-change-silent.test.ts | 41 ++++++++++ .../calendar-store-account-switch.test.ts | 9 ++- .../calendar-store-series-edits.test.ts | 36 ++++++++- src/stores/calendar-store.ts | 34 +++++--- 8 files changed, 237 insertions(+), 19 deletions(-) create mode 100644 src/screens/__tests__/calendar-account-change-silent.test.ts diff --git a/src/components/calendar/EventModal.tsx b/src/components/calendar/EventModal.tsx index bce7fbba..f3041203 100644 --- a/src/components/calendar/EventModal.tsx +++ b/src/components/calendar/EventModal.tsx @@ -45,6 +45,7 @@ import { import { displayNow } from '../../lib/calendar-timezone'; import { editorTimesChanged, editorTimesFromEvent, eventTimeFieldsToSave } from '../../lib/event-editor-times'; import { useSettingsStore } from '../../stores/settings-store'; +import { CalendarAccountChangedError } from '../../stores/calendar-store'; import { canCreateEventsIn } from '../../lib/calendar-editability'; import { useCalendarLocale } from '../../lib/calendar-locale'; import { formatCalendarDate } from '../../lib/date-format'; @@ -417,6 +418,12 @@ export function EventModal({ }); onClose(); } catch (err) { + // The account changed under the save: the event is the previous + // account's, and the calendar shows another one now. + if (err instanceof CalendarAccountChangedError) { + onClose(); + return; + } // The editor stays open with everything typed, and says why. NativeAlert.alert( t('calendar.notifications.event_error', 'Failed to save event'), diff --git a/src/lib/__tests__/recurrence-scope-edit.test.ts b/src/lib/__tests__/recurrence-scope-edit.test.ts index b1c6227e..0075ec60 100644 --- a/src/lib/__tests__/recurrence-scope-edit.test.ts +++ b/src/lib/__tests__/recurrence-scope-edit.test.ts @@ -125,6 +125,25 @@ function actions(master: CalendarEvent | null) { } satisfies OccurrenceEditActions & { deleteEvent: (id: string) => Promise }; } +/** + * `deps` with the account the operation starts in, as the store's + * calendarOperationScope gives it: after `switchAccount` its check throws. + */ +function scoped(deps: D) { + let changed = false; + return { + deps: { + ...deps, + operationScope: () => ({ + assert: () => { + if (changed) throw new Error('The account changed'); + }, + }), + }, + switchAccount: () => { changed = true; }, + }; +} + beforeEach(() => useSettingsStore.setState({ calendarTimeZone: 'Europe/Berlin' })); afterEach(() => useSettingsStore.setState({ calendarTimeZone: 'auto' })); @@ -864,6 +883,39 @@ describe.each(occurrences)('saving this and the following occurrences on Stalwar expect(statusOf(created.participants, ORGANIZER)).toBe('accepted'); expect(statusOf(created.participants, ATTENDEE)).toBe('needs-action'); }); + + // The store writes to the account the client serves when a write begins: + // the rest of the series went to the next account's calendar of the same + // id, and invited its attendees. + it('makes no rest of the series once the account changed after ending it', async () => { + const occurrence = make(); + const { deps, switchAccount } = scoped(actions(series(version))); + deps.updateEvent.mockImplementationOnce(async () => { switchAccount(); return undefined; }); + + await expect(saveOccurrenceEdit('this_and_future', { + event: occurrence, updates: editorUpdates(occurrence), editor: editorOn(occurrence), + calendarId: 'cal', sendScheduling: true, userEmails: [ORGANIZER], + }, deps)).rejects.toThrow('account changed'); + + expect(deps.updateEvent).toHaveBeenCalledTimes(1); + expect(deps.createEvent).not.toHaveBeenCalled(); + }); + + it('puts nothing back in the next account when the account changed while making the rest', async () => { + const occurrence = make(); + const { deps, switchAccount } = scoped(actions(series(version))); + deps.createEvent.mockImplementationOnce(async () => { + switchAccount(); + throw new Error('The account changed while the calendar was saving.'); + }); + + await expect(saveOccurrenceEdit('this_and_future', { + event: occurrence, updates: editorUpdates(occurrence), editor: editorOn(occurrence), sendScheduling: true, userEmails: [ORGANIZER], + }, deps)).rejects.toThrow('account changed'); + + // The truncation alone, no rollback. + expect(deps.updateEvent).toHaveBeenCalledTimes(1); + }); }); describe('the new series from this and the following occurrences', () => { @@ -1156,6 +1208,31 @@ describe.each<[string, Version]>([['1.0', '1.0'], ['0.16', '0.16']])('the series expect(statusOf(change['recurrenceOverrides/2026-10-05T10:00:00'].participants as CalendarEvent['participants'], ATTENDEE)).toBe('accepted'); }); + it('sends no second step once the account changed after the first', async () => { + const master = base(); + const { deps, switchAccount } = scoped(actions(master)); + deps.updateEvent.mockImplementationOnce(async () => { switchAccount(); return undefined; }); + + await expect(saveOccurrenceEdit('this', { + event: master, updates: kickOff(master), sendScheduling: true, userEmails: [ORGANIZER], + }, deps)).rejects.toThrow('account changed'); + + expect(deps.updateEvent).toHaveBeenCalledTimes(1); + expect(deps.updateEvent.mock.calls[0][2]).toEqual({ sendSchedulingMessages: false }); + }); + + it('excludes nothing once the account changed while the series was read', async () => { + const master = base(); + const { deps, switchAccount } = scoped(actions(master)); + deps.getMasterEvent.mockImplementationOnce(async () => { switchAccount(); return master; }); + + await expect(deleteOccurrence('this', { event: master, userEmails: [ORGANIZER] }, deps)).rejects.toThrow('account changed'); + await expect(deleteOccurrence('this_and_future', { event: master, userEmails: [ORGANIZER] }, deps)).rejects.toThrow('account changed'); + + expect(deps.updateEvent).not.toHaveBeenCalled(); + expect(deps.deleteEvent).not.toHaveBeenCalled(); + }); + it('changes it in one write when nobody has answered, or nothing is sent', async () => { const unanswered = base({ participants: { org: participant(version, ORGANIZER, true, 'accepted'), b: participant(version, ATTENDEE, false, 'needs-action') }, diff --git a/src/lib/recurrence-scope-edit.ts b/src/lib/recurrence-scope-edit.ts index 42433cb0..4bab7c73 100644 --- a/src/lib/recurrence-scope-edit.ts +++ b/src/lib/recurrence-scope-edit.ts @@ -581,6 +581,39 @@ export interface OccurrenceEditActions { ) => Promise; /** The base event of a series, from one of its occurrences. */ getMasterEvent: (occurrence: CalendarEvent) => Promise; + /** + * The account the edit is made in, taken once when it starts (the store's + * calendarOperationScope); `assert` throws once the client serves another. + * Each step is a write of its own: after an account switch the next one + * went to the next account with this one's ids - the rest of a split + * series made in its calendar of the same id, mailed to its attendees. + */ + operationScope?: () => { assert: () => void }; +} + +/** + * `actions` with the account checked before every step, against one scope + * for the whole operation: the store writes to the account the client serves + * when a write begins, and a rollback after the switch would undo nothing of + * this account. Steps handed on (a this-and-future edit from the first + * occurrence saves as 'all') keep that scope. + */ +function inOneScope>(actions: A): A { + const scope = actions.operationScope?.(); + if (!scope) return actions; + const checked = Promise>(step: F | undefined) => + step && (((...args: Parameters) => { + scope.assert(); + return step(...args); + }) as unknown as F); + return { + ...actions, + operationScope: () => scope, + updateEvent: checked(actions.updateEvent), + createEvent: checked(actions.createEvent), + deleteEvent: checked(actions.deleteEvent), + getMasterEvent: checked(actions.getMasterEvent), + }; } export interface OccurrenceEdit { @@ -923,8 +956,9 @@ export function followingSeriesData( export async function saveOccurrenceEdit( scope: RecurrenceEditScope, edit: OccurrenceEdit, - actions: OccurrenceEditActions, + unscoped: OccurrenceEditActions, ): Promise { + const actions = inOneScope(unscoped); const { updateEvent, createEvent } = actions; const { event, updates, sendScheduling } = edit; const scheduling = { sendSchedulingMessages: sendScheduling }; @@ -1009,7 +1043,7 @@ export async function saveOccurrenceEdit( } /** What deleting part of a series works with: the calendar store's actions. */ -export interface OccurrenceDeleteActions extends Pick { +export interface OccurrenceDeleteActions extends Pick { deleteEvent: (id: string) => Promise; } @@ -1022,8 +1056,9 @@ export interface OccurrenceDeleteActions extends Pick { + const actions = inOneScope(unscoped); const { event } = deletion; if (scope === 'this') { if (isServerRecurrenceInstance(event) || isBrowserExpandedOccurrence(event)) { diff --git a/src/screens/CalendarScreen.tsx b/src/screens/CalendarScreen.tsx index 6ed8d41b..51c5c672 100644 --- a/src/screens/CalendarScreen.tsx +++ b/src/screens/CalendarScreen.tsx @@ -40,7 +40,7 @@ import { useCalendarLocale } from '../lib/calendar-locale'; import { displayNow } from '../lib/calendar-timezone'; import { spacing, radius, typography, type ThemePalette } from '../theme/tokens'; import { useColors } from '../theme/colors'; -import { useCalendarStore } from '../stores/calendar-store'; +import { CalendarAccountChangedError, calendarOperationScope, useCalendarStore } from '../stores/calendar-store'; import { useSettingsStore } from '../stores/settings-store'; import { MonthView } from '../components/calendar/MonthView'; import { MonthScrollView } from '../components/calendar/MonthScrollView'; @@ -635,6 +635,9 @@ export default function CalendarScreen() { const reportError = React.useCallback( (err: unknown) => { + // The account changed under the write: nothing failed in the account + // shown now, and the previous one shows what got in when it is back. + if (err instanceof CalendarAccountChangedError) return; Alert.alert( t('calendar.notifications.event_error', 'Something went wrong'), err instanceof Error ? err.message : undefined, @@ -724,12 +727,14 @@ export default function CalendarScreen() { sendScheduling: action.sendScheduling, userEmails: currentUserEmails, resolveName: resolveContactName, - }, { updateEvent, createEvent, getMasterEvent }) + }, { updateEvent, createEvent, getMasterEvent, operationScope: calendarOperationScope }) : await deleteOccurrence(scope, { event, userEmails: currentUserEmails }, { - updateEvent, deleteEvent, getMasterEvent, + updateEvent, deleteEvent, getMasterEvent, operationScope: calendarOperationScope, }); if (!done) throw new Error('Master event not found'); } catch (err) { + // The account changed under the edit: it stopped there (see reportError). + if (err instanceof CalendarAccountChangedError) return; Alert.alert( t('calendar.notifications.event_error', 'Something went wrong'), participantLimitMessage(err, t) ?? (err instanceof Error ? err.message : undefined), diff --git a/src/screens/__tests__/calendar-account-change-silent.test.ts b/src/screens/__tests__/calendar-account-change-silent.test.ts new file mode 100644 index 00000000..c1280b62 --- /dev/null +++ b/src/screens/__tests__/calendar-account-change-silent.test.ts @@ -0,0 +1,41 @@ +// A calendar write the account switch overtook throws +// CalendarAccountChangedError: the write stopped so that nothing reaches the +// next account. Nothing failed in the account shown now, and the alert +// "Something went wrong" came up over it. The places that show a calendar +// write's error pass it over in silence. +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const SRC = join(__dirname, '../..'); + +/** The text of `file` from `marker` to the first `Alert.alert(` after it. */ +function beforeAlert(file: string, marker: string): string { + const src = readFileSync(join(SRC, file), 'utf8'); + const start = src.indexOf(marker); + expect(start).toBeGreaterThanOrEqual(0); + const alert = src.indexOf('Alert.alert(', start); + expect(alert).toBeGreaterThan(start); + return src.slice(start, alert); +} + +const SILENT = /if \(err instanceof CalendarAccountChangedError\)/; + +describe('a calendar write the account switch stopped shows no error', () => { + it('CalendarScreen reportError', () => { + expect(beforeAlert('screens/CalendarScreen.tsx', 'const reportError = React.useCallback(')).toMatch(SILENT); + }); + + it('CalendarScreen saving or deleting part of a series', () => { + expect(beforeAlert('screens/CalendarScreen.tsx', "if (!done) throw new Error('Master event not found');")).toMatch(SILENT); + }); + + it('EventModal save', () => { + expect(beforeAlert('components/calendar/EventModal.tsx', 'await onSave(data, calendarId, {')).toMatch(SILENT); + }); + + it('CalendarScreen gives the scope edits one account for the whole operation', () => { + const src = readFileSync(join(SRC, 'screens/CalendarScreen.tsx'), 'utf8'); + expect(src.match(/operationScope: calendarOperationScope/g)).toHaveLength(2); + }); +}); diff --git a/src/stores/__tests__/calendar-store-account-switch.test.ts b/src/stores/__tests__/calendar-store-account-switch.test.ts index 9d46bc84..9881d8ef 100644 --- a/src/stores/__tests__/calendar-store-account-switch.test.ts +++ b/src/stores/__tests__/calendar-store-account-switch.test.ts @@ -99,7 +99,10 @@ describe('a write the account switch overtook', () => { it('names the account it started in and leaves the next account\'s event alone (updateEvent)', async () => { mockUpdate.mockImplementationOnce(async () => switchAccount()); - await useCalendarStore.getState().updateEvent('c', { title: 'Renamed' }); + // Fails, so that a caller with more steps (lib/recurrence-scope-edit) + // sends no next one to the next account. + await expect(useCalendarStore.getState().updateEvent('c', { title: 'Renamed' })) + .rejects.toBeInstanceOf(CalendarAccountChangedError); expect(mockUpdate).toHaveBeenCalledWith('c', { title: 'Renamed' }, undefined, 'acc-1'); expect(useCalendarStore.getState().events).toEqual([THEIRS]); @@ -108,7 +111,7 @@ describe('a write the account switch overtook', () => { it('leaves the next account\'s event of the same id in the store (deleteEvent)', async () => { mockDelete.mockImplementationOnce(async () => switchAccount()); - await useCalendarStore.getState().deleteEvent('c'); + await expect(useCalendarStore.getState().deleteEvent('c')).rejects.toBeInstanceOf(CalendarAccountChangedError); expect(mockDelete).toHaveBeenCalledWith(['c'], undefined, 'acc-1'); expect(useCalendarStore.getState().events).toEqual([THEIRS]); @@ -120,7 +123,7 @@ describe('a write the account switch overtook', () => { return event('n', 'New'); }); - await useCalendarStore.getState().createEvent({ title: 'New' }, 'b'); + await expect(useCalendarStore.getState().createEvent({ title: 'New' }, 'b')).rejects.toBeInstanceOf(CalendarAccountChangedError); expect(mockCreate).toHaveBeenCalledWith({ title: 'New' }, 'b', undefined, 'acc-1'); expect(mockGetEvents).not.toHaveBeenCalled(); diff --git a/src/stores/__tests__/calendar-store-series-edits.test.ts b/src/stores/__tests__/calendar-store-series-edits.test.ts index 32746a22..af775e66 100644 --- a/src/stores/__tests__/calendar-store-series-edits.test.ts +++ b/src/stores/__tests__/calendar-store-series-edits.test.ts @@ -36,7 +36,8 @@ vi.mock('../../api/jmap-client', () => ({ import * as calendarApi from '../../api/calendar'; import type { CalendarEvent } from '../../api/types'; -import { useCalendarStore } from '../calendar-store'; +import { jmapClient } from '../../api/jmap-client'; +import { CalendarAccountChangedError, calendarOperationScope, useCalendarStore } from '../calendar-store'; import { saveOccurrenceEdit } from '../../lib/recurrence-scope-edit'; import { editorTimesFromEvent } from '../../lib/event-editor-times'; @@ -98,6 +99,39 @@ describe('this and the following occurrences on the calendar store (Stalwart 1.0 expect([calendarId, schedule]).toEqual(['cal-1', true]); expect(created).toMatchObject({ start: '2026-10-19T10:00:00', recurrenceRules: [{ '@type': 'RecurrenceRule', frequency: 'weekly', count: 3 }] }); }); + + // The account changes while the store reloads after ending the series: + // that write is done, and the next one took the account the client served + // by then - the rest of the series went to acc-2's calendar of the same id. + it('makes no rest of the series in the account the client serves after a switch', async () => { + const store = useCalendarStore.getState(); + const refresh = store.refresh; + useCalendarStore.setState({ + refresh: async () => { + useCalendarStore.getState().reset(); + (jmapClient as { accountId: string }).accountId = 'acc-2'; + }, + }); + try { + await expect(saveOccurrenceEdit('this_and_future', { + event: occurrence, + updates: { title: 'Planning, room 2', start: occurrence.start, duration: 'PT1H', timeZone: 'Europe/Berlin', participants }, + editor: { attendees: [{ name: '', email: 'userb@example.org' }], when: editorTimesFromEvent(occurrence) }, + calendarId: 'cal-1', sendScheduling: true, userEmails: [ORGANIZER], + }, { + updateEvent: store.updateEvent, createEvent: store.createEvent, getMasterEvent: store.getMasterEvent, + operationScope: calendarOperationScope, + })).rejects.toBeInstanceOf(CalendarAccountChangedError); + } finally { + (jmapClient as { accountId: string }).accountId = 'acc-1'; + useCalendarStore.setState({ refresh }); + } + + // The truncation in acc-1, and nothing after it: no new series, no rollback. + expect(mockUpdate).toHaveBeenCalledTimes(1); + expect(mockUpdate.mock.calls[0][3]).toBe('acc-1'); + expect(mockCreate).not.toHaveBeenCalled(); + }); }); // The series itself, opened from a reminder or a link: not in the grid, and diff --git a/src/stores/calendar-store.ts b/src/stores/calendar-store.ts index 4be8ade0..cfedee56 100644 --- a/src/stores/calendar-store.ts +++ b/src/stores/calendar-store.ts @@ -361,6 +361,19 @@ function assertInScope(scope: WriteScope): void { if (!inScope(scope)) throw new CalendarAccountChangedError(); } +/** + * The account an operation of several store writes runs in (the scope edits + * in lib/recurrence-scope-edit), taken once when it starts. Each write takes + * the account the client serves when it begins, so a step after an account + * switch went to the next account with the ids of this one. `assert` throws + * CalendarAccountChangedError once the store was reset or the client serves + * another account. + */ +export function calendarOperationScope(): { assert: () => void } { + const scope = writeScope(); + return { assert: () => assertInScope(scope) }; +} + /** * The JMAP account a write goes to, named on every request: a shared * calendar's owner, else the primary account the write started in. @@ -1072,8 +1085,9 @@ export const useCalendarStore = create()( schedule, writeAccount(scope, accountId), ); - // Made; the store shows another account by now. - if (!inScope(scope)) return created; + // Made; the store shows another account by now. Throws so that a caller + // with more to do stops here, and opens nothing of the previous account. + assertInScope(scope); // The /set echo lacks server-computed properties (utcStart/utcEnd, the // normalised recurrence rule); re-read the event so it renders at the // right instant, then expand a recurring series across the loaded range @@ -1085,7 +1099,7 @@ export const useCalendarStore = create()( } catch { // Keep the echo; the next refresh reconciles. } - if (!inScope(scope)) return full; + assertInScope(scope); // Map the same way fetchEvents does so the optimistic insert doesn't // collide with the user's own events and stays visible under the right // calendar filter. @@ -1179,8 +1193,9 @@ export const useCalendarStore = create()( } else { await updateBaseEvent(realId, patch, schedule, account, scope); } - // Saved; the store shows another account by now, whose events can carry the same ids. - if (!inScope(scope)) return; + // Saved; the store shows another account by now, whose events can carry + // the same ids. Throws so that a caller with more steps sends no next one. + assertInScope(scope); set({ events: get().events.map((e) => (e.id === id ? { ...e, ...(changes as Partial) } : e)), }); @@ -1206,16 +1221,17 @@ export const useCalendarStore = create()( const schedule = hasSchedulingParticipants(storeEvent) ? true : undefined; if (target.isOccurrence && storeEvent) { await destroyOccurrence(storeEvent, realId, schedule, writeAccount(scope, accountId), scope); - if (!inScope(scope)) return; + assertInScope(scope); set({ events: get().events.filter((e) => e.id !== id) }); } else if (target.isBrowserOccurrence) { await destroyBrowserOccurrence(target, schedule, scope); - if (!inScope(scope)) return; + assertInScope(scope); set({ events: get().events.filter((e) => e.id !== id) }); } else { await apiDeleteEvents([realId], schedule, writeAccount(scope, accountId)); - // Deleted; the store shows another account by now, whose events can carry the same ids. - if (!inScope(scope)) return; + // Deleted; the store shows another account by now, whose events can + // carry the same ids. Throws as updateEvent does. + assertInScope(scope); knownMasters.delete(id); // Destroying a master removes every expanded occurrence of it, not // just the tapped one.