Conversation
📝 WalkthroughWalkthroughImplements end-to-end calendar event reminder delivery. Adds pure ICS alarm computation utilities, a browser Notifications API wrapper, a localStorage deduplication store, and a polling hook ( ChangesVALARM Reminder Delivery
Sequence Diagram(s)sequenceDiagram
participant User
participant EventModal
participant ensureNotificationPermission
participant useEventReminders
participant CalendarAPI
participant showBrowserNotification
participant Toast
participant firedReminderStore
User->>EventModal: Save event with alarms
EventModal->>ensureNotificationPermission: request permission (fire-and-forget)
Note over useEventReminders: Polling loop (fetch interval)
useEventReminders->>CalendarAPI: fetch expanded events
useEventReminders->>CalendarAPI: fetch master events
useEventReminders->>useEventReminders: withMasterAlarms merge
useEventReminders->>firedReminderStore: loadFiredReminders
useEventReminders->>useEventReminders: collectDueReminders
Note over useEventReminders: Tick interval
useEventReminders->>showBrowserNotification: fire(title, body, tag)
useEventReminders->>Toast: fire(title, body)
useEventReminders->>firedReminderStore: saveFiredReminders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/features/calendar/notifications/useEventReminders.tsx`:
- Around line 48-50: The useEffect hook that initializes firedRef.current with
loadFiredReminders(Date.now()) only runs once on component mount, causing
concurrent tabs to have stale fired reminder data that isn't synchronized before
the dedup checks occur around line 84. To fix this, add cross-tab
synchronization by listening to storage events or implementing a mechanism to
refresh firedRef.current whenever the reminder state might have been updated in
another tab. Before performing the dedup checks, call loadFiredReminders again
to ensure firedRef reflects the current state across all tabs, preventing
duplicate notifications from being fired for the same reminder.
- Around line 92-148: Replace the manual polling implementation in
useEventReminders with React Query to align with project standards. Remove the
useCallback for fetchUpcoming and the useEffect that manages setInterval for
fetchTimer and tickTimer, along with the cancelled flag logic. Instead,
implement a useQuery hook that wraps the event fetching logic, using the
refetchInterval option to handle polling at the FETCH_INTERVAL_MS interval, and
remove references to upcomingRef.current by accessing the query data directly
from the useQuery return value. This will delegate interval management, caching,
and lifecycle handling to React Query.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb253b2d-9bde-4ad4-a8a6-1db74bfa9269
📒 Files selected for processing (11)
src/frontend/src/features/calendar/components/scheduler/EventModal.tsxsrc/frontend/src/features/calendar/components/scheduler/hooks/useEventForm.tssrc/frontend/src/features/calendar/components/scheduler/types.tssrc/frontend/src/features/calendar/notifications/NotificationManager.tsxsrc/frontend/src/features/calendar/notifications/__tests__/reminderUtils.test.tssrc/frontend/src/features/calendar/notifications/browserNotifications.tssrc/frontend/src/features/calendar/notifications/firedReminderStore.tssrc/frontend/src/features/calendar/notifications/reminderUtils.tssrc/frontend/src/features/calendar/notifications/useEventReminders.tsxsrc/frontend/src/features/i18n/translations.jsonsrc/frontend/src/routes/index.tsx
Comment on lines
+48
to
+50
| useEffect(() => { | ||
| firedRef.current = loadFiredReminders(Date.now()); | ||
| }, []); |
There was a problem hiding this comment.
Synchronize fired-reminder state across tabs before dedup checks.
Line 48-50 loads firedRef only once, so a concurrently open tab won’t see newly fired keys before Line 84 and can fire duplicate notifications for the same reminder.
Suggested fix
useEffect(() => {
firedRef.current = loadFiredReminders(Date.now());
+ const onStorage = (e: StorageEvent) => {
+ if (e.key === "calendar-fired-reminders") {
+ firedRef.current = loadFiredReminders(Date.now());
+ }
+ };
+ window.addEventListener("storage", onStorage);
+ return () => window.removeEventListener("storage", onStorage);
}, []);Also applies to: 79-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/features/calendar/notifications/useEventReminders.tsx`
around lines 48 - 50, The useEffect hook that initializes firedRef.current with
loadFiredReminders(Date.now()) only runs once on component mount, causing
concurrent tabs to have stale fired reminder data that isn't synchronized before
the dedup checks occur around line 84. To fix this, add cross-tab
synchronization by listening to storage events or implementing a mechanism to
refresh firedRef.current whenever the reminder state might have been updated in
another tab. Before performing the dedup checks, call loadFiredReminders again
to ensure firedRef reflects the current state across all tabs, preventing
duplicate notifications from being fired for the same reminder.
Comment on lines
+92
to
+148
| const fetchUpcoming = useCallback(async () => { | ||
| const urls = [...visibleCalendarUrls]; | ||
| if (urls.length === 0) { | ||
| upcomingRef.current = []; | ||
| return; | ||
| } | ||
| const now = Date.now(); | ||
| const timeRange = { | ||
| start: new Date(now - BACKFILL_MS), | ||
| end: new Date(now + LOOKAHEAD_MS), | ||
| }; | ||
|
|
||
| const fetchCalendar = async (url: string) => { | ||
| // Expanded gives correct per-occurrence start times; non-expanded | ||
| // preserves the master's VALARM (sabre/vobject drops alarms from | ||
| // expanded instances). Merge the two so recurring reminders fire. | ||
| const [expanded, masters] = await Promise.all([ | ||
| caldavService | ||
| .fetchEvents(url, { timeRange, expand: true }) | ||
| .then((res) => (res.success && res.data ? res.data : [])) | ||
| .catch(() => []), | ||
| caldavService | ||
| .fetchEvents(url, { timeRange, expand: false }) | ||
| .then((res) => (res.success && res.data ? res.data : [])) | ||
| .catch(() => []), | ||
| ]); | ||
|
|
||
| const alarmsByUid = buildAlarmMap(masters.flatMap((ce) => ce.data.events ?? [])); | ||
| const instances = expanded.flatMap((ce) => | ||
| (ce.data.events ?? []).map((event) => ({ event, calendarUrl: ce.calendarUrl })), | ||
| ); | ||
| return withMasterAlarms(instances, alarmsByUid); | ||
| }; | ||
|
|
||
| const results = await Promise.all(urls.map(fetchCalendar)); | ||
| upcomingRef.current = results.flat().filter(({ event }) => event.alarms?.length); | ||
| }, [caldavService, visibleCalendarUrls]); | ||
|
|
||
| useEffect(() => { | ||
| if (!isConnected) return; | ||
| let cancelled = false; | ||
|
|
||
| const refetchAndEvaluate = async () => { | ||
| await fetchUpcoming(); | ||
| if (!cancelled) tick(); | ||
| }; | ||
|
|
||
| void refetchAndEvaluate(); | ||
| const fetchTimer = setInterval(() => void refetchAndEvaluate(), FETCH_INTERVAL_MS); | ||
| const tickTimer = setInterval(tick, TICK_INTERVAL_MS); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| clearInterval(fetchTimer); | ||
| clearInterval(tickTimer); | ||
| }; | ||
| }, [isConnected, fetchUpcoming, tick]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read its full content
wc -l src/frontend/src/features/calendar/notifications/useEventReminders.tsxRepository: suitenumerique/calendars
Length of output: 143
🏁 Script executed:
# Check for React Query usage in the frontend codebase
rg -t ts -t tsx "useQuery|useMutation|useInfiniteQuery|React Query" src/frontend/src --max-count=20Repository: suitenumerique/calendars
Length of output: 96
🏁 Script executed:
# Search for setInterval usage in hooks to understand polling patterns
rg -t ts -t tsx "setInterval" src/frontend/srcRepository: suitenumerique/calendars
Length of output: 96
🏁 Script executed:
# Check if React Query is installed as a dependency
cat package.json | grep -i "react-query\|`@tanstack`"Repository: suitenumerique/calendars
Length of output: 113
🏁 Script executed:
# Find package.json files
fd package.jsonRepository: suitenumerique/calendars
Length of output: 115
🏁 Script executed:
# Try searching for React Query with broader patterns
rg "react-query|`@tanstack`|useQuery|useMutation" src/frontend --max-count=20Repository: suitenumerique/calendars
Length of output: 7728
🏁 Script executed:
# Read the full file
cat -n src/frontend/src/features/calendar/notifications/useEventReminders.tsxRepository: suitenumerique/calendars
Length of output: 6753
🏁 Script executed:
# Search for setInterval usage more broadly
rg "setInterval" src/frontendRepository: suitenumerique/calendars
Length of output: 357
Migrate this server-state polling to React Query to align with project standards.
This hook manually manages fetch lifecycle, polling, and cache state for CalDAV events using setInterval. Per project guidelines for src/frontend/src/**/*.{ts,tsx}, implement this using React Query to avoid duplicating query orchestration logic that the library provides.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/features/calendar/notifications/useEventReminders.tsx`
around lines 92 - 148, Replace the manual polling implementation in
useEventReminders with React Query to align with project standards. Remove the
useCallback for fetchUpcoming and the useEffect that manages setInterval for
fetchTimer and tickTimer, along with the cancelled flag logic. Instead,
implement a useQuery hook that wraps the event fetching logic, using the
refetchInterval option to handle polling at the FETCH_INTERVAL_MS interval, and
remove references to upcomingRef.current by accessing the query data directly
from the useQuery return value. This will delegate interval management, caching,
and lifecycle handling to React Query.
Source: Coding guidelines
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.
Fixes #22
Summary by CodeRabbit