Reduce calendar realtime message spam#619
Merged
Conversation
Contributor
WalkthroughAdds two new SQL functions for safer calendar broadcasting and SIS enrollment sync with NULL-safe change detection and guarded DML; and enhances the calendar-sync TypeScript to normalize, trim, and deduplicate ICS events to avoid false-change churn. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CalendarSync as Calendar Sync (TS)
participant DB as Postgres (sync_calendar_events)
participant Broadcaster as Broadcast/Realtime
Client->>CalendarSync: Upload/parse ICS -> list of ParsedEvents
CalendarSync->>CalendarSync: Normalize & deduplicate by UID (countNonNullFields)
CalendarSync->>DB: call sync_calendar_events(p_class_id, calendar_type, parsed_events, has_discord)
DB->>DB: compute adds/updates/deletes using IS DISTINCT FROM
DB->>Broadcaster: emit events for adds/updates/deletes (guarded)
DB-->>CalendarSync: return result JSON (added/updated/deleted/skipped/errors)
CalendarSync-->>Client: return sync result
sequenceDiagram
participant Admin
participant DB as Postgres (sis_sync_enrollment)
participant Invitations as Invitations/Enrollments
participant Emails as Notification/Email
Admin->>DB: call sis_sync_enrollment(p_class_id, roster_data, sync_options)
DB->>DB: authorize admin/instructor, create temp tables, resolve sections
DB->>Invitations: upsert/update enrollments & invitations (conditional)
DB->>Emails: trigger invitation/reactivation notifications as needed
DB-->>Admin: return detailed change/metric JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/calendar-sync/index.ts (1)
835-858:⚠️ Potential issue | 🟠 MajorAvoid date-only keys when overriding occurrences.
Line 837–847 collapses byYYYY-MM-DD, which can drop legitimate same‑day occurrences if multiple instances exist on a single date. Use the full datetime token to match the UID format used in generated occurrences.✅ Proposed fix (align keys with full datetime UID)
- for (const mod of modifiedOccurrences) { - const baseUid = mod.uid; - const dateKey = mod.recurrenceId!.toISOString().split("T")[0]; - modifiedDates.set(`${baseUid}_${dateKey}`, mod); - } + for (const mod of modifiedOccurrences) { + const baseUid = mod.uid; + const dateStr = mod.recurrenceId!.toISOString().replace(/[-:]/g, "").split(".")[0]; + modifiedDates.set(`${baseUid}_${dateStr}`, mod); + } const filteredEvents = expandedEvents.filter((event) => { if (event.recurrenceId) return true; // Keep modified occurrences - const dateKey = event.dtstart.toISOString().split("T")[0]; + const dateKey = event.dtstart.toISOString().replace(/[-:]/g, "").split(".")[0]; const originalUid = getBaseUid(event.uid); const modKey = `${originalUid}_${dateKey}`; return !modifiedDates.has(modKey); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/calendar-sync/index.ts` around lines 835 - 858, The code is using date-only keys (YYYY-MM-DD) when building modifiedDates and when checking modKey, which collapses multiple same-day occurrences; change the keying to use the full datetime token that matches generated UIDs (the same format used later for uid suffixes). Specifically, when iterating modifiedOccurrences build the key using the full recurrence datetime string (e.g., recurrenceId.toISOString().replace(/[-:]/g,"").split(".")[0]) instead of dateKey, and when computing modKey for expandedEvents use getBaseUid(event.uid) + "_" + the same full-datetime token derived from event.dtstart; keep the existing normalization that appends `_${dateStr}` in the final map step so keys and UIDs are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/calendar-sync/index.ts`:
- Around line 998-1020: The file fails Prettier formatting around the
deduplication block; run the project's Prettier/formatter and reformat this file
so it passes CI. Open the section containing parsedEventsRaw, eventsByUid,
countNonNullFields and parsedEvents, fix the long log line and any
trailing/leading whitespace or indentation issues, then save and commit the
reformatted file so the pipeline no longer reports a formatting error.
---
Outside diff comments:
In `@supabase/functions/calendar-sync/index.ts`:
- Around line 835-858: The code is using date-only keys (YYYY-MM-DD) when
building modifiedDates and when checking modKey, which collapses multiple
same-day occurrences; change the keying to use the full datetime token that
matches generated UIDs (the same format used later for uid suffixes).
Specifically, when iterating modifiedOccurrences build the key using the full
recurrence datetime string (e.g.,
recurrenceId.toISOString().replace(/[-:]/g,"").split(".")[0]) instead of
dateKey, and when computing modKey for expandedEvents use getBaseUid(event.uid)
+ "_" + the same full-datetime token derived from event.dtstart; keep the
existing normalization that appends `_${dateStr}` in the final map step so keys
and UIDs are consistent.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/calendar-sync/index.ts (1)
49-60: Minor: Scoring required fields adds no discriminative value.
start_timeandend_timeare always present (non-optional strings fromtoISOString()), so including them in the score adds a constant +2 to every event. The relative comparison still works correctly, but removing them would make the intent clearer—only optional fields actually differentiate events.✨ Optional: Score only optional fields
function countNonNullFields(event: ParsedEvent): number { let score = 0; if (event.title) score++; if (event.description) score++; if (event.location) score++; if (event.queue_name) score++; if (event.organizer_name) score++; - if (event.start_time) score++; - if (event.end_time) score++; return score; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/calendar-sync/index.ts` around lines 49 - 60, The countNonNullFields function currently includes start_time and end_time which are always present and thus add constant value; update countNonNullFields to only count optional/differentiating fields (remove start_time and end_time from the checks) so the score reflects only optional fields like title, description, location, queue_name, and organizer_name; keep the function name countNonNullFields and same return type, simply delete or stop evaluating event.start_time and event.end_time so deduplication scoring is more meaningful.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/calendar-sync/index.ts`:
- Around line 49-60: The countNonNullFields function currently includes
start_time and end_time which are always present and thus add constant value;
update countNonNullFields to only count optional/differentiating fields (remove
start_time and end_time from the checks) so the score reflects only optional
fields like title, description, location, queue_name, and organizer_name; keep
the function name countNonNullFields and same return type, simply delete or stop
evaluating event.start_time and event.end_time so deduplication scoring is more
meaningful.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit