Skip to content

Fix calendar deletion, block edits on read-only calendars and past events - #2797

Merged
manilabui merged 6 commits into
masterfrom
manila/calendar-polish
Aug 13, 2026
Merged

Fix calendar deletion, block edits on read-only calendars and past events#2797
manilabui merged 6 commits into
masterfrom
manila/calendar-polish

Conversation

@manilabui

Copy link
Copy Markdown
Contributor

Summary

Four gaps in how the calendar handles changing and deleting events:

  • The event popover ignored read-only calendars entirely — Edit → change time → Save persisted. Agenda view has no drag gating either, so nothing there stopped it.
  • Delete had no read-only check, and its confirm dialog ran before the guard, so you could confirm a deletion that then silently did nothing.
  • Past events could be dragged or resized, rewriting their time. On a recurring series this wrote a RECURRENCE-ID exception into the master ICS.
  • Deleting an event never persisted. _deleteEntireEvent queued a DestroyModelTask, which the sync engine has no handler for — it hit the else branch at TaskProcessor.cpp:487 ("Unsure of how to process this task type"), marked the task remote, 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 readOnlyCalendarIds set, one used state.calendars.find(...)?.readOnly, which fails open (undefined is falsy) in the window before calendars load.

Fix

  • _isCalendarReadOnly() — one predicate for all four write paths, failing closed until the calendar subscription emits
  • isCalendarReadOnly prop on the popover: no editable view, no edit pencil
  • Delete partitions the selection before prompting, with a showReadOnlyCalendarError() dialog
  • Delete now queues DestroyEventTask.forRemoving({ events: [event] }), which the engine does handle — performLocalDestroyEvent / performRemoteDestroyEvent (TaskProcessor.cpp:484 and :584) already existed and call DAVWorker::deleteEvent. DestroyEventTask was present and exported but had zero call sites; it was orphaned, not dead. Its missing type export was added to mailspring-exports.d.ts.
  • isPastDate() inside canMoveEvent (renamed from canDragEvent, now that the keyboard path calls it too). Cutoff is end < now, so in-progress events stay movable, dragging a future event into the past still works, and popover edits are unaffected.

Behavior changes

  • Cancelled events are no longer arrow-key movable — drag already refused them, and the keyboard path now shares the predicate (see caveat below)
  • Deleting on a read-only calendar shows an error instead of doing nothing

Caveat on the keyboard path

_onMoveSelectedEvent now shares canMoveEvent with 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._onMouseDown calls preventDefault() (only for movable events), so clicking one never focuses it and localHandlers never receive the key; the element remounts on a day-boundary move, destroying focus; and state.selectedEvents is 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 --noEmit and npm run lint — clean
  • npm test — 1471 passing, incl. 8 new specs covering isPastDate and every canMoveEvent branch
  • Manual pass: a past event refuses to drag; an in-progress event still drags (confirming the end < now cutoff 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.
  • Keyboard path only partly verifiable by hand — see the caveat above.

🤖 Generated with Claude Code

@indent-staging

indent-staging Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Warning

Indent Zero is shutting down on August 7th. Please migrate over to Indent 2.0 to continue getting PR reviews.

PR Summary

Polish pass on the calendar plugin: enforces read-only calendars in the event popover and delete flow, blocks time changes on already-ended events via drag/resize/keyboard, consolidates redundant write guards behind a single _isCalendarReadOnly helper on MailspringCalendar, and routes event deletion through DestroyEventTask (which the sync engine recognizes) instead of the generic DestroyModelTask that was previously being silently dropped.

  • CalendarEventPopover gains isCalendarReadOnly prop; edit icon is hidden and renderEditable is suppressed on read-only calendars.
  • canDragEvent renamed to canMoveEvent, now also blocks events whose end time is in the past via new isPastDate helper. All call sites in calendar-event.tsx, month-view-event.tsx, and mailspring-calendar.tsx updated.
  • New _isCalendarReadOnly fails closed until calendarsLoaded flips; _onMoveSelectedEvent, _persistDragChange, and _onDeleteSelectedEvents all route through it. Delete now partitions the selection and short-circuits with showReadOnlyCalendarError() when nothing is deletable.
  • _deleteEntireEvent swaps DestroyModelTask({modelName:'Event',endpoint:'/events',...}) for DestroyEventTask.forRemoving({ events: [event] }); mailspring-exports.d.ts re-exports the task class.
  • New app/spec/calendar-drag-utils-spec.ts covers isPastDate and canMoveEvent (upcoming, ended, in-progress, cancelled, read-only, all-day today vs yesterday).

Issues

1 potential issue found:

  • Popover editor still lets users change times on past events — canMoveEvent's past-end check gates drag/resize/keyboard but the popover's renderEditable only checks isCalendarReadOnly, so opening the edit view on an ended event still exposes the DatePicker/TimePicker and _saveAllOccurrences/_saveOccurrenceException will persist the new times. → Autofix
2 issues already resolved
  • Delete confirmation dialog doesn't disclose partial deletes — when the selection mixes read-only and writable events, the dialog still says "Delete or decline these events?" for the full selection but only the writable subset (deletable) is deleted, so the user has no signal that some events were skipped. (fixed by commit acc78d7)
  • Agenda view bypasses the new fail-closed _isCalendarReadOnly helper — it passes this.props.readOnlyCalendarIds.has(event.calendarId) directly, so during the initial paint before the calendar subscription emits (calendarsLoaded still false, Set still empty) an agenda double-click can open the popover in edit mode for an event on a read-only calendar. (fixed by commit f53a837)

CI Checks

All CI checks passed on commit acc78d7c.

Custom Rules 3 rules evaluated, 3 passed, 0 failed

Passing This is a longer title to see what happens when they are too long to fit
Passing B
Passing Ben Rule

View all rules


⚡ Autofix All Issues

@indent

indent Bot commented Aug 13, 2026

Copy link
Copy Markdown

Issues

Review closed.

CI Checks

All CI checks passed on acc78d7.


render() {
if (this.state.editing || this.props.isNewEvent) {
if (!this.props.isCalendarReadOnly && (this.state.editing || this.props.isNewEvent)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread app/internal_packages/main-calendar/lib/core/agenda-view.tsx Outdated
@manilabui
manilabui merged commit 3b1bd27 into master Aug 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant