Conversation
📝 WalkthroughWalkthroughA guard was added in the new-event insertion path of _sync_events to skip inserting a recurring system event when a matching non-recurring event already exists in the database. This check runs after existing handled/existence checks. No public APIs or signatures were changed. Changes
Sequence Diagram(s)sequenceDiagram
participant SyncEngine
participant SystemCalendar
participant Database
SyncEngine->>SystemCalendar: Fetch system_events
loop For each system_event
SyncEngine->>Database: Check already_handled / already_exists
alt Not handled and not exists
alt system_event.is_recurring and matching non-recurring DB event exists (same start_date, tracking_id, name)
SyncEngine-->>SyncEngine: Skip insertion
else No matching non-recurring DB event
SyncEngine->>Database: Insert new event
end
else
SyncEngine-->>SyncEngine: Skip (already handled/existing)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 1 file. Review in cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/apple-calendar/src/sync.rs (2)
4-4: Remove unused importCalendarSource
CalendarSourceis not referenced anywhere; keep imports clean per guidelines.-use hypr_calendar_interface::{CalendarSource, EventFilter}; +use hypr_calendar_interface::EventFilter;
128-130: Silence/resolve unused tracking set
handled_system_event_idsis only written to and never read, which trips “unused” warnings and adds noise. Either use it to drive creation filtering or mark it intentionally unused.- let mut handled_system_event_ids = std::collections::HashSet::<String>::new(); + let mut _handled_system_event_ids = std::collections::HashSet::<String>::new();Also update the insertion at Line 192:
- handled_system_event_ids.insert(composite_id); + _handled_system_event_ids.insert(composite_id);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/apple-calendar/src/sync.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/apple-calendar/src/sync.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci (macos, macos-latest)
- GitHub Check: ci (windows, windows-latest)
| // don't create a new recurring event if it already exists in the database with the same start date, tracking_id, name and is_recurring | ||
| let already_exists_in_db_recurring = | ||
| db_events_with_session.iter().any(|(db_event, _)| { | ||
| (db_event.start_date == system_event.start_date) | ||
| && (db_event.tracking_id == system_event.id) | ||
| && (system_event.is_recurring) | ||
| && (db_event.name == system_event.name) | ||
| && (db_event.is_recurring == false) | ||
| }); | ||
| if already_exists_in_db_recurring { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid accidental delete-without-recreate: limit the duplicate check to same calendar and ignore rows scheduled for deletion
Current guard can suppress creation of the recurring instance because it sees a matching non-recurring DB row that is also scheduled for deletion earlier in this run, resulting in the event being deleted and not re-created. It also does not constrain the match to the current calendar, which can cause false positives across calendars.
Tighten the predicate as follows:
- Only consider DB rows from the same calendar.
- Ignore DB rows that are already in state.to_delete.
Apply this diff:
- // don't create a new recurring event if it already exists in the database with the same start date, tracking_id, name and is_recurring
- let already_exists_in_db_recurring =
- db_events_with_session.iter().any(|(db_event, _)| {
- (db_event.start_date == system_event.start_date)
- && (db_event.tracking_id == system_event.id)
- && (system_event.is_recurring)
- && (db_event.name == system_event.name)
- && (db_event.is_recurring == false)
- });
+ // Why: avoid duplicating an event when a prior non-recurring record exists (e.g., source flipped to recurring).
+ let already_exists_in_db_recurring =
+ db_events_with_session.iter().any(|(db_event, _)| {
+ system_event.is_recurring
+ && !db_event.is_recurring
+ && db_event.start_date == system_event.start_date
+ && db_event.tracking_id == system_event.id
+ && db_event.name == system_event.name
+ // ensure we're matching within the same calendar
+ && db_event.calendar_id.as_deref() == Some(db_calendar.id.as_str())
+ // ignore rows that are already scheduled for deletion in this sync run
+ && !state.to_delete.iter().any(|e| e.id == db_event.id)
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // don't create a new recurring event if it already exists in the database with the same start date, tracking_id, name and is_recurring | |
| let already_exists_in_db_recurring = | |
| db_events_with_session.iter().any(|(db_event, _)| { | |
| (db_event.start_date == system_event.start_date) | |
| && (db_event.tracking_id == system_event.id) | |
| && (system_event.is_recurring) | |
| && (db_event.name == system_event.name) | |
| && (db_event.is_recurring == false) | |
| }); | |
| if already_exists_in_db_recurring { | |
| continue; | |
| } | |
| // Why: avoid duplicating an event when a prior non-recurring record exists (e.g., source flipped to recurring). | |
| let already_exists_in_db_recurring = | |
| db_events_with_session.iter().any(|(db_event, _)| { | |
| system_event.is_recurring | |
| && !db_event.is_recurring | |
| && db_event.start_date == system_event.start_date | |
| && db_event.tracking_id == system_event.id | |
| && db_event.name == system_event.name | |
| // ensure we're matching within the same calendar | |
| && db_event.calendar_id.as_deref() == Some(db_calendar.id.as_str()) | |
| // ignore rows that are already scheduled for deletion in this sync run | |
| && !state.to_delete.iter().any(|e| e.id == db_event.id) | |
| }); | |
| if already_exists_in_db_recurring { | |
| continue; | |
| } |
🤖 Prompt for AI Agents
In plugins/apple-calendar/src/sync.rs around lines 272 to 284, the
duplicate-check predicate is too broad and can match rows from other calendars
or rows already scheduled for deletion; update the closure to destructure the
pair (db_event, state) and only consider matches where db_event.calendar_id
equals system_event.calendar_id and the DB row is not marked for deletion
(state.to_delete is false / !state.to_delete), keeping the existing checks for
start_date, tracking_id, name and is_recurring; this prevents false positives
across calendars and avoids skipping create when the matching row is going to be
deleted.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/apple-calendar/src/sync.rs (1)
272-282: Tighten duplicate-check to same calendar and ignore rows scheduled for deletion; drop session gatingAs written, this predicate can:
- Skip creation due to a matching row in a different calendar (false positive).
- Skip creation even when the matching row is already scheduled for deletion earlier in this run (delete-without-recreate).
- Miss matches when the legacy row has no session (Line 281-282).
Constrain by calendar_id and ignore rows in state.to_delete. Also remove the session gating; it’s orthogonal to deduping and causes misses.
Apply this diff:
- // don't create a new recurring event if it already exists in the database with the same start date, tracking_id, name and is_recurring - // only for backward compatibility with the old database structure - let already_exists_in_db_recurring = - db_events_with_session.iter().any(|(db_event, session)| { - (db_event.start_date == system_event.start_date) - && (db_event.tracking_id == system_event.id) - && (system_event.is_recurring) - && (db_event.name == system_event.name) - && (db_event.is_recurring == false) - && session.as_ref().map_or(false, |s| !s.is_empty()) - }); + // Why: prevent duplicating a recurring event when a legacy non-recurring row exists. + // Constrain to same calendar and ignore rows scheduled for deletion in this sync run. + let already_exists_in_db_recurring = + db_events_with_session.iter().any(|(db_event, _)| { + system_event.is_recurring + && !db_event.is_recurring + && db_event.start_date == system_event.start_date + && db_event.tracking_id == system_event.id + && db_event.name == system_event.name + && db_event + .calendar_id + .as_deref() + == Some(db_calendar.id.as_str()) + && !state.to_delete.iter().any(|e| e.id == db_event.id) + });
🧹 Nitpick comments (1)
plugins/apple-calendar/src/sync.rs (1)
283-285: Add debug trace for the skip pathWhen this guard triggers, it’s hard to reason about why the event wasn’t created. Add a debug log to aid troubleshooting.
- if already_exists_in_db_recurring { - continue; - } + if already_exists_in_db_recurring { + tracing::debug!( + "skip_create_due_to_legacy_nonrecurring: sys_event_id={} cal_id={}", + system_event.id, + db_calendar.id + ); + continue; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/apple-calendar/src/sync.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit Configuration File
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
plugins/apple-calendar/src/sync.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (1)
plugins/apple-calendar/src/sync.rs (1)
276-276: Confirm start_date equality is stable across zones/granularityDirect equality on DateTime can be brittle if DB and system events differ in timezone normalization or precision. If the Apple API returns local times that you normalize to UTC elsewhere, this is fine; otherwise consider normalizing before comparison or comparing at minute precision.
1 participant
Summary by cubic
Added a check to prevent creating duplicate recurring events in Apple Calendar sync when a matching non-recurring event already exists.