Fix calendar deletion, block edits on read-only calendars and past events - #2797
Conversation
|
Warning Indent Zero is shutting down on August 7th. Please migrate over to Indent 2.0 to continue getting PR reviews.
|
|
Review closed.
All CI checks passed on |
|
|
||
| render() { | ||
| if (this.state.editing || this.props.isNewEvent) { | ||
| if (!this.props.isCalendarReadOnly && (this.state.editing || this.props.isNewEvent)) { |
There was a problem hiding this comment.
Past events remain time-editable through the popover. canMoveEvent now blocks drag / resize / keyboard nudges once event.end is in the past, but this render check only consults isCalendarReadOnly. Clicking the edit icon on an ended event still opens renderEditable, exposing the start/end DatePicker + TimePicker, and _saveAllOccurrences / _saveOccurrenceException will happily persist the new times. If the commit message intent ("Block time changes to events that have already ended") is meant to cover the popover too, gate this branch on canMoveEvent(event, isCalendarReadOnly) and hide the edit icon in CalendarEventPopoverUnenditable for past events. If the intent is drag/keyboard-only, worth noting in the commit — right now the enforcement is one-sided.
There was a problem hiding this comment.
Intentional — the past-event rule covers direct manipulation only (drag, resize, arrow keys), not the popover.
The reasoning: those three are gestures you can trigger by accident, and a nudge that silently rewrites a past event is hard to notice. Opening the popover, clicking edit, changing a time and hitting Save is deliberate, and it is the escape hatch for legitimately correcting a past event — so gating it would leave no way to fix an event after the fact.
Noted in the commit body for d6b11b887 ("Popover edits are unaffected") and in the PR description under Fix, though both are easy to miss. Happy to make it more prominent.
Your second comment is a real bug and is being fixed — agenda was reading the raw readOnlyCalendarIds Set instead of the fail-closed check, which matters more than it looks because the popover render gate is now the only enforcement on that path.
Summary
Four gaps in how the calendar handles changing and deleting events:
RECURRENCE-IDexception into the master ICS._deleteEntireEventqueued aDestroyModelTask, which the sync engine has no handler for — it hit theelsebranch atTaskProcessor.cpp:487("Unsure of how to process this task type"), marked the taskremote, and deleted nothing. The event vanished locally, then the next sync restored it from the server.The four read-only checks also disagreed with each other: three used the
readOnlyCalendarIdsset, one usedstate.calendars.find(...)?.readOnly, which fails open (undefinedis falsy) in the window before calendars load.Fix
_isCalendarReadOnly()— one predicate for all four write paths, failing closed until the calendar subscription emitsisCalendarReadOnlyprop on the popover: no editable view, no edit pencilshowReadOnlyCalendarError()dialogDestroyEventTask.forRemoving({ events: [event] }), which the engine does handle —performLocalDestroyEvent/performRemoteDestroyEvent(TaskProcessor.cpp:484and:584) already existed and callDAVWorker::deleteEvent.DestroyEventTaskwas present and exported but had zero call sites; it was orphaned, not dead. Its missing type export was added tomailspring-exports.d.ts.isPastDate()insidecanMoveEvent(renamed fromcanDragEvent, now that the keyboard path calls it too). Cutoff isend < now, so in-progress events stay movable, dragging a future event into the past still works, and popover edits are unaffected.Behavior changes
Caveat on the keyboard path
_onMoveSelectedEventnow sharescanMoveEventwith drag, so read-only/past/cancelled are refused there too. But the arrow-key shortcuts are hard to reach at all today, for reasons that predate this branch:CalendarEvent._onMouseDowncallspreventDefault()(only for movable events), so clicking one never focuses it andlocalHandlersnever receive the key; the element remounts on a day-boundary move, destroying focus; andstate.selectedEventsis never refreshed after a change, so repeated presses recompute from a stale position. Tracked separately — the guard here is correct but only partly demonstrable by hand.Test plan
npx tsc -p ./app --noEmitandnpm run lint— cleannpm test— 1471 passing, incl. 8 new specs coveringisPastDateand everycanMoveEventbranchend < nowcutoff doesn't over-block); a read-only calendar's popover has no edit pencil; deleting via Menu → Calendar → Delete Event persists through a sync instead of reappearing.🤖 Generated with Claude Code