Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for repeated (recurring) calendar items end-to-end: UI for creating/editing recurrence rules, persistence/schema changes, recurrence expansion when querying items, and improved iCal RRULE import/export.
Changes:
- Add repeat configuration UI to the calendar item dialog and persist repeating items as
RepeatingCalendarItem. - Expand repeating calendar items into occurrences when fetching calendar items for a date/range, and bump DB schema version with a v5 migration.
- Improve iCal RRULE parsing/serialization to include BYDAY/BYMONTHDAY variations, and adjust UI list keys to differentiate occurrences.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/lib/pages/calendar/pending.dart | Updates list tile keys to include start/end for repeated occurrences. |
| app/lib/pages/calendar/month.dart | Updates day dialog list tile keys to include occurrence start/end. |
| app/lib/pages/calendar/item.dart | Adds repeat rule editing UI and converts dialog state into persisted repeating items on save. |
| app/lib/pages/calendar/day.dart | Adjusts internal resize keying to account for repeated occurrences (start/end). |
| app/lib/api/storage/remote/caldav.dart | Formatting-only changes and minor cleanup around calendar object updates. |
| api/lib/services/migration.dart | Adds DB migration to v5 for updated calendarItems runtimeType handling. |
| api/lib/services/database.dart | Bumps database version to 5. |
| api/lib/models/user/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/resource/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/note/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/note/database.dart | Formatting-only changes in linker methods. |
| api/lib/models/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/label/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/group/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/extra.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/event/model.mapper.dart | Generated mapper update (new ignore). |
| api/lib/models/event/item/model.mapper.dart | Updates generated calendar item mapping; removes AutoCalendarItem mapper. |
| api/lib/models/event/item/model.dart | Adds exceptions decoding, bitmask helpers for BYDAY/BYMONTHDAY, and DB (de)serialization tweaks. |
| api/lib/models/event/item/database.dart | Implements recurrence expansion for repeating items in getCalendarItems and updates schema (drops auto* columns). |
| api/lib/models/cached.mapper.dart | Adjusts generated copyWith typing/behavior for calendar items. |
| api/lib/converters/ical.dart | Adds BYDAY/BYMONTHDAY parsing and RRULE formatting with variation support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key: ValueKey([ | ||
| item.source, | ||
| item.main.id, | ||
| item.main.start?.millisecondsSinceEpoch, | ||
| item.main.end?.millisecondsSinceEpoch, | ||
| ]), |
There was a problem hiding this comment.
ValueKey is being constructed with a new List instance on every build. Since List equality in Dart is identity-based, the key will effectively always be different even for the same occurrence, which can cause unnecessary rebuilds and loss of element state. Use a key value with stable equality (e.g., a single String via .join('@') / interpolation, or a small value object/record whose fields also have value-based equality).
| key: ValueKey([ | |
| item.source, | |
| item.main.id, | |
| item.main.start?.millisecondsSinceEpoch, | |
| item.main.end?.millisecondsSinceEpoch, | |
| ]), | |
| key: ValueKey<String>( | |
| '${item.source}@${item.main.id}@${item.main.start?.millisecondsSinceEpoch}@${item.main.end?.millisecondsSinceEpoch}', | |
| ), |
| final occurrenceKey = [ | ||
| e.source, | ||
| e.main.id, | ||
| e.main.start?.millisecondsSinceEpoch, | ||
| e.main.end?.millisecondsSinceEpoch, | ||
| ]; |
There was a problem hiding this comment.
occurrenceKey is a List used inside ValueKey. Because List equality is identity-based, rebuilding this widget will generate a different key each time even if the contents are unchanged, breaking element identity/state. Consider using a stable String key (e.g., by joining the parts) or another value with proper ==/hashCode.
| final occurrenceKey = [ | |
| e.source, | |
| e.main.id, | |
| e.main.start?.millisecondsSinceEpoch, | |
| e.main.end?.millisecondsSinceEpoch, | |
| ]; | |
| final occurrenceKey = | |
| '${e.source}|${e.main.id}|${e.main.start?.millisecondsSinceEpoch}|${e.main.end?.millisecondsSinceEpoch}'; |
| String _repeatTypeLabel(BuildContext context, RepeatType type) { | ||
| return switch (type) { | ||
| RepeatType.daily => 'Daily', | ||
| RepeatType.weekly => 'Weekly', | ||
| RepeatType.monthly => 'Monthly', | ||
| RepeatType.yearly => 'Yearly', | ||
| }; | ||
| } | ||
|
|
||
| String _weekdayLabel(int weekday) { | ||
| return const { | ||
| DateTime.monday: 'Mon', | ||
| DateTime.tuesday: 'Tue', | ||
| DateTime.wednesday: 'Wed', | ||
| DateTime.thursday: 'Thu', | ||
| DateTime.friday: 'Fri', | ||
| DateTime.saturday: 'Sat', | ||
| DateTime.sunday: 'Sun', | ||
| }[weekday]!; |
There was a problem hiding this comment.
These repeat type labels are hard-coded English strings (and the context parameter is currently unused). Since this dialog already uses AppLocalizations elsewhere, these should be localized (and ideally derived from localized weekday/month strings) to avoid regressions for non-English locales.
| if (type != CalendarItemType.pending) ...[ | ||
| const SizedBox(height: 16), | ||
| CheckboxListTile( | ||
| title: const Text('Repeat'), | ||
| value: _isRepeating, |
There was a problem hiding this comment.
The newly added repeating UI uses multiple hard-coded strings (e.g., 'Repeat', 'Frequency', 'Interval', helper text). This dialog otherwise uses AppLocalizations, so these should be moved to the localization bundle to keep the UI translatable and consistent.
| factory CalendarItem.fromDatabase(Map<String, dynamic> row) { | ||
| final mapped = {...row, 'exceptions': _decodeExceptions(row['exceptions'])}; | ||
| final runtimeType = row['runtimeType']?.toString(); | ||
| if (runtimeType == 'RepeatingCalendarItem' || runtimeType == 'repeating') { |
There was a problem hiding this comment.
CalendarItem.fromDatabase treats only RepeatingCalendarItem / repeating as repeating. Elsewhere (e.g., recurrence queries/migration) AutoCalendarItem / auto are still considered repeating legacy types. To avoid mis-decoding legacy rows (or partially migrated data), include those runtimeType values here as well (or ensure they cannot reach this code path).
| if (runtimeType == 'RepeatingCalendarItem' || runtimeType == 'repeating') { | |
| if (runtimeType == 'RepeatingCalendarItem' || | |
| runtimeType == 'repeating' || | |
| runtimeType == 'AutoCalendarItem' || | |
| runtimeType == 'auto') { |
| await db.transaction((txn) async { | ||
| await service.calendarItem.create(txn, 'calendarItems_temp_v5'); | ||
| await txn.execute( | ||
| "INSERT INTO calendarItems_temp_v5 " |
There was a problem hiding this comment.
This adds a v5 migration that recreates calendarItems without the removed autoGroupId/searchStart/autoDuration columns, but the existing <4 migration earlier in this file still does INSERT INTO calendarItems_temp SELECT ... autoGroupId, searchStart, autoDuration .... With the updated calendarItems schema (see CalendarItemDatabaseService.create), upgrading directly from DB versions <4 will fail due to a column-count mismatch. Update the <4 migration to insert only the columns that exist in the new schema (preferably by specifying an explicit column list).
| "INSERT INTO calendarItems_temp_v5 " | |
| "INSERT INTO calendarItems_temp_v5 (" | |
| "runtimeType, id, name, description, location, eventId, start, end, " | |
| "status, repeatType, interval, variation, count, until, exceptions" | |
| ") " |
No description provided.