fix: outlook notification showing event time in the wrong timezone #37318
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: b438de0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughServer adds Changes
Sequence DiagramsequenceDiagram
actor User
participant Server
participant Client
participant DB as CalendarEventDB
Server->>DB: fetch event for notification
Server->>Server: build payload (include startTimeUtc = event.startTime.toISOString())
Server->>Client: send notification with payload
alt payload contains startTimeUtc
Client->>Client: parse startTimeUtc → format to locale time (HH:MM + dayPeriod)
Client->>Client: set notification body to formatted time
Client->>User: display enhanced notification
else no startTimeUtc or format error
Client->>User: display original notification text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37318 +/- ##
===========================================
- Coverage 67.93% 67.91% -0.03%
===========================================
Files 3356 3356
Lines 114886 114894 +8
Branches 20758 20755 -3
===========================================
- Hits 78049 78031 -18
- Misses 34152 34175 +23
- Partials 2685 2688 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/services/calendar/service.ts (1)
353-360: Critical: Server-side time formatting conflicts with client-side timezone correction.Line 355 formats the time using the server's locale and timezone by calling
toLocaleTimeString(undefined, ...). When the client receives this notification, it appends the correctly formatted user-timezone time (from line 358'sstartTimeUtc), resulting in a notification body with two conflicting times.Example scenario:
- Event at 2025-01-15T14:00:00Z
- Server (UTC): formats as "2:00 PM"
- Client (EST): formats as "9:00 AM"
- Final notification: "2:00 PM - 9:00 AM" ❌
Recommended fix:
Since the client now handles timezone-aware formatting using
startTimeUtc, remove the time formatting from the server-sidetextfield:return api.broadcast('notify.calendar', event.uid, { title: event.subject, - text: event.startTime.toLocaleTimeString(undefined, { hour: 'numeric', minute: 'numeric', dayPeriod: 'narrow' }), + text: event.subject, payload: { _id: event._id, startTimeUtc: event.startTime.toISOString(), }, });Alternatively, if you want to preserve backwards compatibility for clients without the
startTimeUtchandling, conditionally format:text: event.subject, // Client will append time when startTimeUtc is presentThis ensures the client properly displays: "Meeting Title at 9:00 AM" in the user's local timezone.
🧹 Nitpick comments (1)
apps/meteor/client/views/root/hooks/loggedIn/useNotificationUserCalendar.ts (1)
28-31: Consider adding the event ID to the error log for debugging.While the error handling is correct, including the event ID would help trace issues for specific calendar events.
} catch (error) { - console.error('Failed to format calendar notification time:', error); + console.error(`Failed to format calendar notification time for event ${notification.payload._id}:`, error); body = notification.text; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/client/views/root/hooks/loggedIn/useNotificationUserCalendar.ts(1 hunks)apps/meteor/server/services/calendar/service.ts(1 hunks)packages/core-typings/src/INotification.ts(1 hunks)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/core-typings/src/INotification.ts (1)
77-77: LGTM: Type definition correctly supports the timezone fix.The optional
startTimeUtcfield properly enables the client to format notifications in the user's timezone while maintaining backwards compatibility.
apps/meteor/client/views/root/hooks/loggedIn/useNotificationUserCalendar.ts
Show resolved
Hide resolved
jeanfbrito
added
the
stat: QA assured
scuciatto
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
scuciatto
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
…37318) Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
What Was Happening
Users were receiving calendar event notifications from Outlook/Exchange that displayed event times in the wrong timezone.
For example, if you had a meeting scheduled for 2:00 PM in New York, the notification might show it as 6:00 PM UTC or some other incorrect time, making it confusing to know when you actually needed to attend because it was using the server's timezone.
What's Now Fixed
The calendar notification system now properly converts event times to display in each user's local timezone.
Examples of the fix:
This ensures that when you receive a calendar notification, the time displayed matches your local timezone, eliminating confusion about when your events are actually happening, regardless of where you're located in the world, based on your machine's timezone.
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-902
Steps to test or reproduce
Further comments
Summary by CodeRabbit