-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(calendar): correct day-of-week for full-day recurring events across all timezones #4004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Is the a matching edge case with one hour close to |
|
I initially thought this was DST-related too, but after testing I just updated the PR text - it's not about DST. The bug affects all full-day events in western timezones (UTC-1 to UTC-12), regardless of DST: // Western timezones: Always wrong
moment.tz(new Date("2025-11-03T00:00:00Z"), "America/Chicago")
// → 2025-11-02 18:00 → .startOf("day") → 2025-11-02 ❌
// Eastern timezones: Always correct
moment.tz(new Date("2025-11-03T00:00:00Z"), "Asia/Tokyo")
// → 2025-11-03 09:00 → .startOf("day") → 2025-11-03 ✅No matching edge case exists - the bug is asymmetric. UTC midnight converts to the previous day in western timezones, but same day in eastern timezones. That's why eastern timezones work correctly by design. |
…ss DST Full-day recurring events were showing on wrong day after DST transitions. rrule.js returns UTC dates that shift calendar days when offset changes. Fix: Extract UTC date components and interpret as local calendar dates, preserving the intended day regardless of DST offset changes. Added test for weekly Monday event crossing DST boundary. Fixes: MagicMirrorOrg#4003 Regression from: c2ec6fc
e37df6d to
4dda8c3
Compare
|
I just fixed a comment that still referred to DST. No changes in the logic. |
|
I'm too tired to continue researching right now, but after further investigation, it seems to me that the old logic before c2ec6fc is not as wrong as I described it above 🤯. But that doesn't change the fact that we need a fix (this PR) now. I hope that we switch to Temporal at some point, then all the time zone stuff will be a little less confusing. |
|
No rush. Not shipping til April 1 |
|
User reports PR fixed issue |
|
Great 🙂 Then we can merge it, right? |
|
@sdetweil Is the problem serious enough that we should do a fix release? 🤔 |
|
no, we can teach them how to get the PR |
|
Okay, I think that's a good call 👍 |
Fixes full-day recurring events showing on wrong day in timezones west of UTC (reported in #4003).
Root cause:
moment.tz(date, eventTimezone).startOf("day")interprets UTC midnight as local time:2025-11-03T00:00:00.000Zin America/Chicago (UTC-6)2025-11-02 18:00:00(6 hours back).startOf("day")→2025-11-02 00:00:00❌ Wrong day!Impact: The bug affects:
The issue was introduced with commit c2ec6fc (#3976), which fixed the time but broke the date. This PR fixes both.
2025-11-03 05:00:00 Monday2025-11-02 00:00:00 Sunday✅ (east of UTC)
2025-11-03 00:00:00 MondayNote: While the old logic had incorrect timing, it produced the correct calendar day due to how it handled UTC offsets. The current logic fixed the timing issue but introduced the more visible calendar day bug.
Solution
Extract UTC date components and interpret as local calendar dates:
Testing
To prevent this from happening again in future refactorings, I wrote a test for it.
npm test -- tests/unit/modules/default/calendar/calendar_fetcher_utils_spec.js