Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds an is_recurring boolean across calendar interfaces, providers, DB schema, seeds, and JS bindings; Apple Calendar sync now distinguishes recurring instances via composite tracking IDs and preserves tracking_id on updates while disabling reschedule handling. Desktop calendar header now starts on Monday. Changes
Sequence Diagram(s)sequenceDiagram
participant AppleCalendar as Apple Calendar (system)
participant Plugin as Apple Calendar Plugin
participant DB as DB (events)
AppleCalendar->>Plugin: list_events() (events include recurrence info)
Plugin->>Plugin: compute is_recurring for each event
alt recurring
Plugin->>Plugin: composite_tracking_id = id__HYPR__yyyymmddHHMMSS
else non-recurring
Plugin->>Plugin: composite_tracking_id = id
end
Plugin->>DB: find by composite_tracking_id
alt found
Plugin->>DB: update event (preserve existing tracking_id, update fields incl. is_recurring)
else not found
Plugin->>DB: insert event (tracking_id = composite_tracking_id, set is_recurring)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✨ 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
5 issues found across 9 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| let new_event = hypr_db_user::Event { | ||
| id: uuid::Uuid::new_v4().to_string(), | ||
| tracking_id: system_event.id.clone(), | ||
| tracking_id: composite_tracking_id, |
There was a problem hiding this comment.
Backward-incompatible change to recurring event tracking_id causes mass deletions/duplicates of existing events
Prompt for AI agents
Address the following comment on plugins/apple-calendar/src/sync.rs at line 275:
<comment>Backward-incompatible change to recurring event tracking_id causes mass deletions/duplicates of existing events</comment>
<file context>
@@ -213,27 +242,37 @@ async fn _sync_events(
for db_calendar in db_selected_calendars {
if let Some(fresh_events) = system_events_per_selected_calendar.get(&db_calendar.id) {
for system_event in fresh_events {
+ let composite_tracking_id = if system_event.is_recurring {
+ format!(
+ "{}__HYPR__{}",
+ system_event.id,
+ system_event.start_date.format("%Y%m%d%H%M%S")
+ )
</file context>
| pub start_date: DateTime<Utc>, | ||
| pub end_date: DateTime<Utc>, | ||
| pub google_event_url: Option<String>, | ||
| pub is_recurring: bool, |
There was a problem hiding this comment.
Adding a new non-optional field to a serde-deserializable struct will break deserialization of existing data that doesn’t include this field. Provide a default or make it optional to maintain backward compatibility.
Prompt for AI agents
Address the following comment on crates/calendar-interface/src/lib.rs at line 48:
<comment>Adding a new non-optional field to a serde-deserializable struct will break deserialization of existing data that doesn’t include this field. Provide a default or make it optional to maintain backward compatibility.</comment>
<file context>
@@ -45,6 +45,7 @@ pub struct Event {
pub start_date: DateTime<Utc>,
pub end_date: DateTime<Utc>,
pub google_event_url: Option<String>,
+ pub is_recurring: bool,
}
</file context>
| pub is_recurring: bool, | |
| #[serde(default)] pub is_recurring: bool, |
| }; | ||
|
|
||
| const weekDays = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"]; | ||
| const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; |
There was a problem hiding this comment.
Weekday labels are hard-coded in English and the week start is fixed to Monday, which ignores user locale. Use locale-aware formatting for day names and derive the first day of week from the active locale to ensure correct labels for non-English users and regions that start weeks on Sunday.
Prompt for AI agents
Address the following comment on apps/desktop/src/components/finder/views/calendar-view.tsx at line 40:
<comment>Weekday labels are hard-coded in English and the week start is fixed to Monday, which ignores user locale. Use locale-aware formatting for day names and derive the first day of week from the active locale to ensure correct labels for non-English users and regions that start weeks on Sunday.</comment>
<file context>
@@ -37,7 +37,7 @@ export function CalendarView({ date, sessions, events, onNavigate }: CalendarVie
onNavigate({ date: today.toISOString() });
};
- const weekDays = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"];
+ const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"];
return (
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
crates/db-user/src/lib.rs (1)
132-155: Migration ordering violates “Append only. Do not reorder.” Move the new migration to the end.You inserted events_migration_2.sql before the previous last entry. If your migrator applies by ordinal index, existing databases with N migrations applied may attempt to re-run the previously last migration or apply out of order. Append-only means adding the new migration as the last element.
Apply this diff to preserve ordering and append the new migration:
- include_str!("./events_migration_1.sql"), - include_str!("./events_migration_2.sql"), - include_str!("./session_participants_migration_1.sql"), + include_str!("./events_migration_1.sql"), + include_str!("./session_participants_migration_1.sql"), + include_str!("./events_migration_2.sql"),MIGRATIONS length [22] remains correct.
🧹 Nitpick comments (3)
crates/db-user/src/init.rs (1)
81-82: Consistent initialization looks good; consider a tiny helper to reduce repetitionAll seed/onboarding Events explicitly set is_recurring: false, which is consistent with the new schema. To reduce repetition and future drift, consider a small constructor/helper for seed data that defaults is_recurring to false (and fills other common fields), so you only override when needed.
I can draft a minimal helper pattern that fits this module if you’d like.
Also applies to: 245-245, 259-259, 273-273, 287-287, 301-301, 315-315, 341-341, 355-355, 379-379, 405-405, 419-419, 446-446, 460-460, 486-486, 500-500, 515-515, 529-529, 543-543, 557-557
plugins/apple-calendar/src/sync.rs (2)
156-167: Update path correctly preserves DB tracking_id and syncs fields.Matching by composite ID for recurring instances and preserving the existing tracking_id avoids churn. Looks good.
If you want to reduce duplication, consider extracting composite ID computation into a small helper used across update/new paths (example provided below in another comment).
245-254: Composite ID construction is repeated; consider a small helper for consistency.To avoid drift between update/creation/logging paths, a single composite_tracking_id(event) helper reduces duplication and mistakes.
Outside this hunk, you can add:
fn composite_tracking_id(e: &hypr_calendar_interface::Event) -> String { if e.is_recurring { format!("{}__HYPR__{}", e.id, e.start_date.format("%Y%m%d%H%M%S")) } else { e.id.clone() } }Then replace in-place constructions with composite_tracking_id(&system_event)/composite_tracking_id(matching_event).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/components/finder/views/calendar-view.tsx(1 hunks)crates/calendar-apple/src/lib.rs(2 hunks)crates/calendar-google/src/lib.rs(1 hunks)crates/calendar-interface/src/lib.rs(1 hunks)crates/db-user/src/events_migration_2.sql(1 hunks)crates/db-user/src/events_types.rs(1 hunks)crates/db-user/src/init.rs(20 hunks)crates/db-user/src/lib.rs(2 hunks)plugins/apple-calendar/src/sync.rs(10 hunks)plugins/db/js/bindings.gen.ts(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:
crates/calendar-interface/src/lib.rscrates/db-user/src/events_types.rsapps/desktop/src/components/finder/views/calendar-view.tsxcrates/calendar-apple/src/lib.rsplugins/db/js/bindings.gen.tscrates/db-user/src/init.rscrates/db-user/src/lib.rscrates/calendar-google/src/lib.rsplugins/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 (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (5)
crates/calendar-apple/src/lib.rs (1)
256-257: Propagating is_recurring looks correct.The field is added in the Event construction and reflects the computed recurrence state. Good alignment with the updated interface.
crates/calendar-google/src/lib.rs (1)
107-107: Explicitly setting is_recurring to false is acceptable for now.This matches the stated intent for the Google provider in this hotfix.
plugins/apple-calendar/src/sync.rs (2)
265-268: DB existence check by composite tracking_id is correct.This will dedupe against legacy rows (non-recurring) and new recurring instances (composite).
275-288: Propagating is_recurring on creation is correct.New rows carry the recurrence flag consistently.
plugins/db/js/bindings.gen.ts (1)
171-171: TS bindings aligned: Event now includes is_recurring.This mirrors the schema and Rust types across the stack. Good.
| }; | ||
|
|
||
| const weekDays = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"]; | ||
| const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Week starts on Monday: verify grid/body alignment and consider locale-aware labels
Switching to Monday-first is fine, but please verify WorkspaceCalendar renders days in the same order to avoid header/body misalignment. Also consider deriving labels from locale instead of hard-coded English abbreviations.
Run this to locate potential Sunday-first assumptions and duplicated weekday arrays:
Optionally, replace the static array with a locale-aware, Monday-first calculation:
- const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"];
+ const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; // TODO(why): Hard-coded for hotfix; consider locale-aware EEE with weekStartsOn=1.If you want the full locale-aware refactor, I can provide a minimal diff using date-fns format/startOfWeek.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect WorkspaceCalendar implementation and any weekday-related logic
fd -t f 'workspace-calendar' | xargs -I{} sh -c 'echo "== {} =="; rg -n "week|weekday|startOfWeek|Sun|Mon|getDay" "{}" || true'
# Search for hard-coded weekday arrays across the repo
rg -n --glob '!node_modules' --glob '!*dist*' --glob '!*build*' --glob '!*target*' '"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"|weekDays'Length of output: 763
Fix weekday-order mismatch — calendar header is Monday-first but shared util is Sunday-first
Found conflicting weekday arrays — this will risk header/body misalignment. Unify to a single source-of-truth and prefer locale-aware labels.
Files to check
- apps/desktop/src/components/finder/views/calendar-view.tsx
- Line ~40:
const weekDays = ["Mon","Tue","Wed","Thu","Fri","Sat","Sun"]; - Used at lines ~78 and ~83 to render header cells.
- Line ~40:
- packages/utils/src/datetime.ts
- Line ~105:
const daysOfWeek = ["Sun","Mon","Tue","Wed","Thu","Fri","Sat"];(Sunday-first)
- Line ~105:
Actionable suggestions
- Replace hard-coded arrays with a single exported util (e.g., getWeekDays({ weekStartsOn, locale })) in packages/utils and import it in calendar-view, or generate locale-aware short names via Intl.DateTimeFormat.
- Ensure the calendar body (any code that maps dates to columns / uses getDay/getDay index) uses the same week start (weekStartsOn) so header and grid align.
Minimal inline replacement example (calendar-view.tsx)
- const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"];
+ // locale-aware, Monday-first (example using Intl)
+ const weekDays = (() => {
+ const fmt = new Intl.DateTimeFormat(undefined, { weekday: "short" });
+ const monday = new Date(Date.UTC(2020, 0, 6)); // known Monday anchor
+ return Array.from({ length: 7 }).map((_, i) => fmt.format(new Date(monday.getTime() + i * 86400000)));
+ })();If you want, I can provide a PR-ready refactor that either (a) exports a locale-aware getter from packages/utils/src/datetime.ts or (b) uses date-fns (format/startOfWeek) to standardize weekStartsOn across the codebase.
📝 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.
| const weekDays = ["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]; | |
| // locale-aware, Monday-first (example using Intl) | |
| const weekDays = (() => { | |
| const fmt = new Intl.DateTimeFormat(undefined, { weekday: "short" }); | |
| const monday = new Date(Date.UTC(2020, 0, 6)); // known Monday anchor | |
| return Array.from({ length: 7 }).map((_, i) => fmt.format(new Date(monday.getTime() + i * 86400000))); | |
| })(); |
🤖 Prompt for AI Agents
In apps/desktop/src/components/finder/views/calendar-view.tsx around line 40,
the file defines a hard-coded Monday-first weekDays array that conflicts with
packages/utils/src/datetime.ts which is Sunday-first; replace the local array
with a single source-of-truth: import and call a shared utility (e.g.,
getWeekDays({ weekStartsOn, locale })) from packages/utils (or generate short
weekday names via Intl.DateTimeFormat with the same weekStartsOn), use that
returned array to render header cells, and update any calendar body logic that
maps dates to columns to apply the same weekStartsOn offset (normalize getDay()
indices using the weekStartsOn) so header and grid align.
| // experiment: check if the event is recurring | ||
| let is_recurring = unsafe { | ||
| let has_rules: Bool = msg_send![event, hasRecurrenceRules]; | ||
| has_rules.as_bool() | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Recurrence detection: confirm API availability and trim the comment (use “why”).
Calling hasRecurrenceRules via msg_send is reasonable, but please confirm it’s available across the minimum iOS/macOS targets you support. If any target lacks this selector, consider a fallback check using recurrenceRules != nil.
Also, the comment “experiment” doesn’t convey why we’re doing this. Replace with a short “why”.
🌐 Web query:
Is EKEvent.hasRecurrenceRules documented and available across supported iOS/macOS versions? Provide official Apple docs and minimum availability.
💡 Result:
EKEvent.hasRecurrenceRules is not an officially documented property by Apple for EventKit; it does not appear in Apple’s EventKit class reference or official documentation, and therefore its cross-platform or minimum version availability cannot be confirmed from authoritative Apple sources.
Official Apple Documentation
- Apple’s official API documentation for EKEvent and its superclass EKCalendarItem document the
recurrenceRulesproperty, which is an array ofEKRecurrenceRuleobjects, and methods to add or remove recurrence rules, but there is no mention of ahasRecurrenceRulesproperty[4]. - Official EventKit documentation recommends checking if an event is recurring by querying the
recurrenceRulesarray—if it is non-empty, the event is recurring[4][5][7].
Availability and Platform Version
- Because
hasRecurrenceRulesis not official, there is no stated minimum OS version for this property in either iOS or macOS. - The standard way across all supported versions (as far back as iOS 4 and macOS 10.8)[1][5] is to use
recurrenceRules:if (event.recurrenceRules != nil && event.recurrenceRules.count > 0) { /* event is recurring */ }
Context in the Ecosystem
- Mentions of
hasRecurrenceRulesare sometimes seen in third-party bindings, community tutorials, or language wrappers (such as Xamarin or .NET auto-generated docs)[10], but these are not authoritative, and typically reflect helpers or computed properties rather than Apple-defined API surface.
Summary Table
| Property | Apple-official? | Min iOS version | Min macOS version |
|---|---|---|---|
EKEvent.hasRecurrenceRules |
No | N/A | N/A |
EKEvent.recurrenceRules |
Yes | iOS 4.0+ | OS X 10.8+ |
References to recurrence rules in Apple documentation always use the recurrenceRules array and never a hasRecurrenceRules Boolean property.[4][7][5]
To check for recurring events, use event.recurrenceRules.count > 0, which is available and supported across all Apple platforms with EventKit support.
If you need confirmation from official Apple Developer documentation, see the recurrenceRules property of EKCalendarItem[4].
Citations:
- 1: https://rcconsulting.com/Downloads/EventKitProgGuide.pdf
- 2: https://github.com/rmaddy/RWMRecurrenceRule
- 3: https://developer.apple.com/documentation/eventkit/ekcalendar/supportedeventavailabilities
- 4: https://developer.apple.com/documentation/eventkit/ekcalendaritem/recurrencerules?language=objc
- 5: https://www.apeth.com/iOSBook/ch32.html
- 6: https://developer.apple.com/documentation/eventkit/ekeventavailability
- 7: https://developer.apple.com/documentation/eventkit/creating-a-recurring-event
- 8: https://nemecek.be/blog/35/how-to-create-ios-reminders-in-code-with-alarms-or-recurrences
- 9: https://developer.apple.com/documentation/eventkit/ekevent/availability
- 10: https://learn.microsoft.com/en-us/dotnet/api/eventkit.ekevent?view=xamarin-ios-sdk-12
Recurrence detection: use documented recurrenceRules and clarify the comment (why).
EKEvent.hasRecurrenceRules is not an Apple-documented API and its availability cannot be relied on. Use the documented recurrenceRules array (check for nil / count > 0) and replace the vague "experiment" comment with a short "why".
- File: crates/calendar-apple/src/lib.rs — lines 229–234
Suggested replacement snippet:
// why: use the documented `recurrenceRules` to determine if an event is recurring
let is_recurring = unsafe {
let rules: *mut objc::runtime::Object = msg_send![event, recurrenceRules];
if rules.is_null() {
false
} else {
let count: usize = msg_send![rules, count];
count > 0
}
};Apply this change (or at minimum check recurrenceRules != nil) and remove the use of the non‑official hasRecurrenceRules selector.
🤖 Prompt for AI Agents
In crates/calendar-apple/src/lib.rs around lines 229 to 234, replace the unsafe
call to the non‑documented selector hasRecurrenceRules with a documented check
of recurrenceRules: send the recurrenceRules selector, test for null, and if
non‑null call count and treat the event as recurring when count > 0; also
replace the vague "experiment" comment with a brief "why: use the documented
`recurrenceRules` to determine if an event is recurring" and remove any use of
the non‑official selector.
| ALTER TABLE | ||
| events | ||
| ADD | ||
| COLUMN is_recurring BOOLEAN DEFAULT FALSE; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the new column NOT NULL with a safe default (backward-compatible and type-safe)
As written, existing rows may end up with NULLs depending on engine/version behavior. Given the Rust type is bool (non-Option), enforce NOT NULL and set a default. Recommended:
- Simple, robust approach (preferred for correctness):
-ALTER TABLE
- events
-ADD
- COLUMN is_recurring BOOLEAN DEFAULT FALSE;
+ALTER TABLE
+ events
+ADD
+ COLUMN is_recurring BOOLEAN NOT NULL DEFAULT FALSE;- If you want to minimize table rewrite on some Postgres versions, use a 3-step approach:
- ADD COLUMN nullable,
- UPDATE existing rows to FALSE,
- ALTER COLUMN SET NOT NULL and SET DEFAULT FALSE.
📝 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.
| ALTER TABLE | |
| events | |
| ADD | |
| COLUMN is_recurring BOOLEAN DEFAULT FALSE; | |
| ALTER TABLE | |
| events | |
| ADD | |
| COLUMN is_recurring BOOLEAN NOT NULL DEFAULT FALSE; |
🤖 Prompt for AI Agents
In crates/db-user/src/events_migration_2.sql around lines 1 to 4, the new
is_recurring column is added without NOT NULL which can produce NULLs
incompatible with the Rust bool type; modify the migration to make the column
NOT NULL with a safe default by either adding it as: ADD COLUMN is_recurring
BOOLEAN NOT NULL DEFAULT FALSE, or (to avoid table rewrite on some Postgres
versions) perform the 3-step safe approach: 1) ADD COLUMN is_recurring BOOLEAN
DEFAULT FALSE (nullable), 2) UPDATE events SET is_recurring = FALSE WHERE
is_recurring IS NULL, then 3) ALTER TABLE events ALTER COLUMN is_recurring SET
NOT NULL and ALTER COLUMN is_recurring SET DEFAULT FALSE.
| pub is_recurring: bool, | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New non-optional bool requires DB non-null guarantee
Adding a non-optional bool is good, but it must be backed by a NOT NULL column (with a default) to avoid deserialization failures when reading legacy rows.
- Ensure the migration sets is_recurring NOT NULL DEFAULT FALSE and backfills existing records. See my comment on events_migration_2.sql for the exact change.
- If there’s any chance of ingesting older serialized Events (e.g., cached JSON without this field), consider adding a serde default to avoid breakage:
- pub is_recurring: bool,
+ #[serde(default)]
+ pub is_recurring: bool,Note: Confirm user_common_derives! passes field attributes through as-is.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for occurrences of is_recurring ==="
rg --hidden -n "is_recurring" || true
echo
echo "=== Show file crates/db-user/src/events_types.rs (if exists) ==="
if [ -f "crates/db-user/src/events_types.rs" ]; then
sed -n '1,240p' crates/db-user/src/events_types.rs || true
else
echo "File not found: crates/db-user/src/events_types.rs"
fi
echo
echo "=== Search for migrations mentioning events or is_recurring ==="
rg --hidden -n "events_migration|events_migration_2.sql|events_migration_2|ALTER TABLE .*is_recurring|is_recurring.*NOT NULL|is_recurring.*DEFAULT" || true
echo
echo "=== Search for user_common_derives macro definition/usages ==="
rg --hidden -n "user_common_derives" || true
echo
echo "=== Try to show macro definition(s) for user_common_derives ==="
# Find files that contain the macro_rules or proc macro
rg --hidden -n "macro_rules! user_common_derives|proc_macro.*user_common_derives|fn user_common_derives" || true
# If found, print nearby context for each file
FILES=$(rg --hidden -n "user_common_derives" -l || true)
if [ -n "$FILES" ]; then
for f in $FILES; do
echo
echo "---- $f ----"
sed -n '1,240p' "$f" || true
done
else
echo "No files containing user_common_derives found."
fi
echo
echo "=== Search for serde(default) usages ==="
rg --hidden -n "#\\[serde\\(default\\)\\]|serde\\(default\\)" || trueLength of output: 31353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== events_migration_2.sql ==="
if [ -f "crates/db-user/src/events_migration_2.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration_2.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration_2.sql"
fi
echo
echo "=== events_migration_1.sql ==="
if [ -f "crates/db-user/src/events_migration_1.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration_1.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration_1.sql"
fi
echo
echo "=== events_migration.sql ==="
if [ -f "crates/db-user/src/events_migration.sql" ]; then
sed -n '1,240p' crates/db-user/src/events_migration.sql
else
echo "NOT FOUND: crates/db-user/src/events_migration.sql"
fi
echo
echo "=== crates/db-user/src/events_ops.rs ==="
if [ -f "crates/db-user/src/events_ops.rs" ]; then
sed -n '1,240p' crates/db-user/src/events_ops.rs
else
echo "NOT FOUND: crates/db-user/src/events_ops.rs"
fi
echo
echo "=== crates/db-user/src/init.rs (relevant insert lines) ==="
if [ -f "crates/db-user/src/init.rs" ]; then
sed -n '1,240p' crates/db-user/src/init.rs | nl -ba | sed -n '1,240p'
else
echo "NOT FOUND: crates/db-user/src/init.rs"
fiLength of output: 10034
Fix DB + serde + ops for new non-optional is_recurring field
Short: the repo already added the column with DEFAULT FALSE, but the migration, deserialization safety, and DB persistence need fixes so the non-optional bool can't cause runtime/serialization bugs.
Files to change
- crates/db-user/src/events_migration_2.sql
- Current:
ADD COLUMN is_recurring BOOLEAN DEFAULT FALSE; - Change to ensure NOT NULL and backfill existing rows. Example:
ALTER TABLE events ADD COLUMN is_recurring BOOLEAN NOT NULL DEFAULT FALSE; -- if needed (or as a separate migration): UPDATE events SET is_recurring = FALSE WHERE is_recurring IS NULL;
- Current:
- crates/db-user/src/events_types.rs
- Add serde default to tolerate older serialized data:
- pub is_recurring: bool, + #[serde(default)]
- Add serde default to tolerate older serialized data:
-
pub is_recurring: bool,- (Confirmed) user_common_derives! preserves field-level attributes, so the above will be emitted as-is.
- crates/db-user/src/events_ops.rs
- Persist the field in INSERT/UPDATE (currently INSERT/UPDATE omit
is_recurring, so callers' values are ignored). Example diffs:- INSERT: add column/value and param
- ..., google_event_url, participants + ..., google_event_url, participants, is_recurring
- VALUES: add
:is_recurring - ON CONFLICT DO UPDATE SET: include
is_recurring = :is_recurring - Update the named_params maps to include:
+ ":is_recurring": event.is_recurring, - UPDATE query: add
is_recurring = :is_recurringand the named param.
- Persist the field in INSERT/UPDATE (currently INSERT/UPDATE omit
Notes
- Migration currently includes DEFAULT FALSE (crates/db-user/src/events_migration_2.sql) but not NOT NULL — ensure migrations enforce NOT NULL (or add a follow-up migration that backfills and then enforces NOT NULL) to avoid any NULLs when deserializing into a non-optional bool.
- Adding #[serde(default)] is recommended defensive programming for any legacy JSON/cached serialized Events.
- Also verify tests/seeders that construct Event include the new field where necessary (many seeds already set is_recurring: false).
🤖 Prompt for AI Agents
In crates/db-user/src/events_types.rs around lines 17-18, the new non-optional
field is_recurring lacks a serde default attribute and will fail when
deserializing older data; add #[serde(default)] (or #[serde(default = "false")]
equivalent) to the is_recurring field so missing/NULL values deserialize to
false. Also update crates/db-user/src/events_migration_2.sql to add NOT NULL and
backfill existing rows (ALTER TABLE events ADD COLUMN is_recurring BOOLEAN NOT
NULL DEFAULT FALSE; or run UPDATE events SET is_recurring = FALSE WHERE
is_recurring IS NULL; then ALTER ... NOT NULL), and modify
crates/db-user/src/events_ops.rs to include is_recurring in INSERT and UPDATE
statements (add the column to INSERT, add :is_recurring to VALUES and ON
CONFLICT DO UPDATE SET, and include is_recurring = :is_recurring in UPDATE) and
add the named param to all named_params maps so callers’ values are persisted.
| let composite_id = if matching_event.is_recurring { | ||
| format!( | ||
| "{}__HYPR__{}", | ||
| matching_event.id, | ||
| matching_event.start_date.format("%Y%m%d%H%M%S") | ||
| ) | ||
| } else { | ||
| matching_event.id.clone() | ||
| }; | ||
| // Mark this system event as handled | ||
| handled_system_event_ids.insert(matching_event.id.clone()); | ||
| handled_system_event_ids.insert(composite_id); | ||
| continue; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Write-only set: handled_system_event_ids is never consulted. Use it to prevent duplicate creations.
You insert composite IDs into handled_system_event_ids but never read it. This defeats the intent of “prevent duplicates”. Either remove the set or use it in the creation loop to skip already-handled instances.
Apply this diff to consult the set before upserting new events:
for system_event in fresh_events {
let composite_tracking_id = if system_event.is_recurring {
format!(
"{}__HYPR__{}",
system_event.id,
system_event.start_date.format("%Y%m%d%H%M%S")
)
} else {
system_event.id.clone()
};
// Skip if this event was already handled as an update
- let already_handled = state
- .to_update
- .iter()
- .any(|e| e.tracking_id == composite_tracking_id);
+ let already_handled = state
+ .to_update
+ .iter()
+ .any(|e| e.tracking_id == composite_tracking_id)
+ // Also skip if we explicitly marked it handled
+ || handled_system_event_ids.contains(&composite_tracking_id);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/apple-calendar/src/sync.rs around lines 182 to 193, the HashSet
handled_system_event_ids is populated but never consulted, so it fails to
prevent duplicate creations; update the event creation/upsert loop to compute
the same composite_id for each candidate system event and check
handled_system_event_ids.contains(&composite_id) before creating/upserting—if
present, skip that item (continue); keep the existing insert when you mark a
system event as handled, or alternatively remove the set entirely if you choose
not to deduplicate, but do not leave it write-only.
| // Skip if this event was already handled as an update | ||
| let already_handled = state | ||
| .to_update | ||
| .iter() | ||
| .any(|e| e.tracking_id == system_event.id); | ||
| .any(|e| e.tracking_id == composite_tracking_id); | ||
| if already_handled { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Leverage handled_system_event_ids when skipping already-handled instances.
With the write-only set fixed (previous comment), this condition now covers both “updated” and “explicitly marked handled” events, preventing duplicate upserts for recurring instances.
The diff to consult the set is included in the earlier comment (Lines 182-193).
🤖 Prompt for AI Agents
In plugins/apple-calendar/src/sync.rs around lines 255 to 260, the skip logic
only checks state.to_update for matching composite_tracking_id; update this
condition to also consult the write-only handled_system_event_ids set (see
earlier diff at lines 182-193) so that the branch treats events as already
handled if either an update entry matches OR handled_system_event_ids contains
the composite_tracking_id. Ensure you reference the correct set name/type and
use handled_system_event_ids.contains(&composite_tracking_id) (or the equivalent
method) combined with the existing any(...) check.
| tracing::info!( | ||
| "System event: '{}' | participants: {:?}", | ||
| "System event: '{}' | participants: {:?} | is_recurring: {} | start_date: {} | tracking_id: {}", | ||
| event.name, | ||
| event.participants | ||
| event.participants, | ||
| event.is_recurring, | ||
| event.start_date, | ||
| event.id, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Log shows “tracking_id” but prints system id. Log the composite tracking_id instead.
The current log prints event.id labeled as tracking_id, which is misleading for recurring events. Compute and log the composite tracking_id for clarity.
Apply this diff:
- for event in &events {
- tracing::info!(
- "System event: '{}' | participants: {:?} | is_recurring: {} | start_date: {} | tracking_id: {}",
- event.name,
- event.participants,
- event.is_recurring,
- event.start_date,
- event.id,
- );
- }
+ for event in &events {
+ let tracking_id = if event.is_recurring {
+ format!("{}__HYPR__{}", event.id, event.start_date.format("%Y%m%d%H%M%S"))
+ } else {
+ event.id.clone()
+ };
+ tracing::info!(
+ "System event: '{}' | participants: {:?} | is_recurring: {} | start_date: {} | tracking_id: {}",
+ event.name,
+ event.participants,
+ event.is_recurring,
+ event.start_date,
+ tracking_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.
| tracing::info!( | |
| "System event: '{}' | participants: {:?}", | |
| "System event: '{}' | participants: {:?} | is_recurring: {} | start_date: {} | tracking_id: {}", | |
| event.name, | |
| event.participants | |
| event.participants, | |
| event.is_recurring, | |
| event.start_date, | |
| event.id, | |
| ); | |
| for event in &events { | |
| let tracking_id = if event.is_recurring { | |
| format!("{}__HYPR__{}", event.id, event.start_date.format("%Y%m%d%H%M%S")) | |
| } else { | |
| event.id.clone() | |
| }; | |
| tracing::info!( | |
| "System event: '{}' | participants: {:?} | is_recurring: {} | start_date: {} | tracking_id: {}", | |
| event.name, | |
| event.participants, | |
| event.is_recurring, | |
| event.start_date, | |
| tracking_id, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In plugins/apple-calendar/src/sync.rs around lines 386 to 393, the log labels
"tracking_id" but currently prints event.id; compute the composite tracking_id
for recurring events (i.e., combine the system id with the recurrence/instance
identifier or use the event.tracking_id accessor if present) and store it in a
local variable, then replace event.id in the tracing::info! call with that
composite tracking_id so logs reflect the real tracking identifier for both
single and recurring events.
2 participants
Summary by cubic
Added recurring event support for calendar events and improved handling of event syncing in the Apple Calendar integration.
is_recurringflag.