fix: resolve DST bug in sunrise/sunset trigger timing (#3737) #3754
Conversation
The original implementation of sunrise/sunset event timing had a critical DST bug where event times would jump 1+ hours across DST transitions (issue bitfocus#3737). The root cause was using getTimezoneOffset() in day-of-year calculations, which changes during DST transitions. ## Key Changes ### DST Fix Implementation (Timer.ts) - Replaced timezone-offset-based day calculation with dayjs for robust calendar math - Changed time creation from local timezone to UTC-based Date.UTC() approach - Applied offset in UTC minutes instead of local minutes to avoid DST issues - This ensures sunrise/sunset times remain stable across DST boundaries ### Testing - Added comprehensive test suite with 536 tests covering: - 20 geographic locations across 6 continents (20 locations × 10 years × 2 event types = 400 baseline tests) - DST transitions in both spring and fall for multiple hemispheres - Non-DST timezones (Hawaii, Arizona, Iceland, Moscow, India, Nepal) - Edge cases: leap years, year boundaries, polar regions, equinoxes/solstices - Offset precision: positive/negative offsets, boundary values, extreme values - Multiple Timer instances, idempotency, concurrent updates - Invalid inputs: NaN, Infinity, extreme coordinates (±90°, ±180°) ### Debug Logging - Added informative debug logging on setSun() with: - Event type (Sunrise/Sunset) - Location coordinates (latitude, longitude) - Offset in minutes - Next scheduled time in both local and UTC formats ### New Test Categories - Offset Precision Near DST Boundaries - Multiple Independent Timer Instances - Large Negative Offset Edge Cases - Absolute Idempotency with State Verification - Maximum Valid Coordinate Precision 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The two tests documenting the DST bug (bitfocus#3737) now pass with the fix in place. Rename them to remove the "FAILING TEST" prefix for clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
|
|
I'll admit I've not looked through the 3.5k lines of tests, and I'm not sure I could tell you what the correct behaviour should actually be, but have you got a test for when the offset to sunset/sunrise takes it into the magic DST hour that doesn't exist/exists twice depending on which way the clock is going? |
|
Assuming I understand your question, the tests you are looking for are in the "DST Boundary Jump Verification" test at line 1226. |
I was meaning more "should handle offset that causes day wraparound during DST" but with an offset that takes it into the repeated/dropped hour depending on the DST direction. |
peternewman
left a comment
peternewman
left a comment
There was a problem hiding this comment.
I'm going to assume the tests were the main part written by AI?
I'd argue a lot of those test cases should be a lot stricter, even if it's just to confirm that the edge case behaviour remains as is...
I've also only reviewed ones with an offset set (~170), not all the others that exist.
| // Day before DST (March 29, 2025) - CET (UTC+1) | ||
| vi.setSystemTime(new Date('2025-03-29T12:00:00Z')) | ||
| timer.setSun('pre-dst-berlin', params) | ||
| const preTime = timer.getSunNextExecuteTime('pre-dst-berlin') |
There was a problem hiding this comment.
According to https://www.timeanddate.com/sun/germany/berlin?month=3&year=2025 this should be 18:35
| // Day after DST (March 30, 2025) - CEST (UTC+2) | ||
| vi.setSystemTime(new Date('2025-03-30T12:00:00Z')) | ||
| timer.setSun('post-dst-berlin', params) | ||
| const postTime = timer.getSunNextExecuteTime('post-dst-berlin') |
There was a problem hiding this comment.
Likewise this should be 19:37 (as the CET to CEST offset is 1 hour).
|
|
||
| // Local time should be approximately the same | ||
| const hourDifference = Math.abs(postLocalHour - preLocalHour) | ||
| expect(hourDifference).toBeLessThan(3) |
There was a problem hiding this comment.
Shouldn't this therefore be checking its only changed by the DST offset?:
| expect(hourDifference).toBeLessThan(3) | |
| expect(hourDifference).toBeLessThan(2) |
Or are you also trying to cover the edge case place where the sunset was say 18:59 beforehand and hence would be 20:01 afterwards (~2 minutes of movement for the day, and then an hour of clock change)?
Wouldn't it be better to avoid that edge case by not picking test cases which sit near an hour boundary, or (and this perhaps brings it's own issues) doing maths on the hours and minutes combined, where the total move would be the clock change and ~5 minutes?
There was a problem hiding this comment.
Should you also add another test to confirm you've caught the right day too (again assuming it hasn't also tipped over an hour change during the test:
| expect(hourDifference).toBeLessThan(3) | |
| expect(hourDifference).toBeLessThan(3) | |
| expect(hourDifference).toBeGreaterThan(1) |
Your current test would be true on any (non-clock change) day of the year too.
|
|
||
| // Local time should be approximately the same | ||
| const hourDifference = Math.abs(postLocalHour - preLocalHour) | ||
| expect(hourDifference).toBeLessThan(3) |
| } | ||
|
|
||
| // Day before DST ends (April 6, 2025) - AEDT (UTC+11) | ||
| vi.setSystemTime(new Date('2025-04-06T12:00:00Z')) |
There was a problem hiding this comment.
This has the clock change happening 5th into 6th April in Sydney:
https://www.timeanddate.com/sun/australia/sydney?month=4&year=2025
Assuming your offset is due to picking UTC rather than local, you ought to add a big here be dragons comment warning.
| }) | ||
|
|
||
| describe('Maximum Valid Coordinate Precision', () => { | ||
| test('latitude +90 (north pole) should calculate or return NaN', () => { |
There was a problem hiding this comment.
Which one is it, calculate or return NaN?
| expect(typeof time === 'number' || time === null).toBe(true) | ||
| }) | ||
|
|
||
| test('latitude -90 (south pole) should calculate or return NaN', () => { |
| expect(time).not.toBeNull() | ||
| expect(typeof time).toBe('number') | ||
| expect(time).toBeGreaterThan(0) |
| timer.setSun(`corner-${corner.name}`, params) | ||
| const time = timer.getSunNextExecuteTime(`corner-${corner.name}`) | ||
|
|
||
| expect(typeof time === 'number' || time === null).toBe(true) |
| expect(time).not.toBeNull() | ||
| expect(typeof time).toBe('number') | ||
| expect(time).toBeGreaterThan(0) |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical DST bug (issue #3737) in sunrise/sunset trigger timing where event times would incorrectly jump 1+ hours across DST transitions. The root cause was using getTimezoneOffset() in day-of-year calculations, which changes during DST transitions.
Key Changes:
- Replaced timezone-offset-based day calculation with dayjs for robust calendar math
- Changed time creation from local timezone to UTC-based
Date.UTC()approach to avoid DST issues - Applied offset in UTC minutes instead of local minutes
- Added comprehensive test suite with 536 tests covering multiple geographic locations, DST transitions, and edge cases
- Added debug logging for sun event scheduling
- Added
getSunNextExecuteTime()method for testing purposes
Reviewed Changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| companion/lib/Controls/ControlTypes/Triggers/Events/Timer.ts | Implemented DST fix by using dayjs for day-of-year calculation and UTC-based date creation; added debug logging and test helper method |
| companion/test/Controls/Triggers/Events/Timer.test.ts | Added comprehensive test suite with 536 tests covering DST transitions, multiple timezones, edge cases, and validation scenarios |
Comments suppressed due to low confidence (1)
companion/lib/Controls/ControlTypes/Triggers/Events/Timer.ts:537
- The comment here says "Remove a timeofday event listener" but this method is actually removing a sun event listener. This should be updated to "Remove a sun event listener" to match what the method actually does.
/**
* Remove a timeofday event listener
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // UT is the UTC time as decimal hours when sunset occurs | ||
| const utHours = Math.floor(UT) | ||
| const utMinutes = Math.floor((UT - utHours) * 60) | ||
|
|
||
| const sunEventTime = new Date(ms) | ||
| sunEventTime.setFullYear(now.getFullYear()) | ||
| sunEventTime.setMonth(now.getMonth()) | ||
| sunEventTime.setDate(now.getDate()) | ||
| // Get the UTC date that corresponds to this local date | ||
| const localDateAtMidnight = new Date(now.getFullYear(), now.getMonth(), now.getDate()) | ||
| const utcYear = localDateAtMidnight.getUTCFullYear() | ||
| const utcMonth = localDateAtMidnight.getUTCMonth() | ||
| const utcDate = localDateAtMidnight.getUTCDate() | ||
|
|
||
| const temp_minutes = sunEventTime.getMinutes() | ||
| // Create the date at the UTC time using Date.UTC | ||
| const sunEventTime = new Date(Date.UTC(utcYear, utcMonth, utcDate, utHours, utMinutes, 0)) |
There was a problem hiding this comment.
[nitpick] There's a subtle issue with the seconds component in the UTC time calculation. Line 353 sets seconds to 0, but the original calculation produces a fractional number of minutes in UT. When converting UT to hours and minutes on lines 343-344, the fractional seconds are lost due to Math.floor().
For more precise timing, consider:
const utSeconds = Math.floor(((UT - utHours) * 60 - utMinutes) * 60)
const sunEventTime = new Date(Date.UTC(utcYear, utcMonth, utcDate, utHours, utMinutes, utSeconds))This preserves the precision of the astronomical calculation rather than truncating to the nearest minute. While the difference is small (up to 59 seconds), for a sun event trigger this could matter for user expectations.
| // Convert to local time strings for comparison | ||
| const preDate = new Date(preTime!) | ||
| const postDate = new Date(postTime!) | ||
| const preLocalHour = preDate.getHours() | ||
| const postLocalHour = postDate.getHours() | ||
|
|
||
| // Local time should be approximately the same (within 2 hours due to seasonal drift) | ||
| // If there's a 1-hour bug, the times would differ by more than 2 hours | ||
| const hourDifference = Math.abs(postLocalHour - preLocalHour) | ||
| expect(hourDifference).toBeLessThan(3) | ||
| } finally { | ||
| vi.useRealTimers() | ||
| } | ||
| }) | ||
|
|
||
| test('Europe (Berlin): sunset should not jump 1 hour on DST spring forward (2025-03-29 to 2025-03-30)', () => { | ||
| vi.useFakeTimers() | ||
| try { | ||
| const params = { | ||
| type: 'sunset', | ||
| latitude: 52.52, | ||
| longitude: 13.41, // Berlin | ||
| offset: 0, | ||
| } | ||
|
|
||
| // Day before DST (March 29, 2025) - CET (UTC+1) | ||
| vi.setSystemTime(new Date('2025-03-29T12:00:00Z')) | ||
| timer.setSun('pre-dst-berlin', params) | ||
| const preTime = timer.getSunNextExecuteTime('pre-dst-berlin') | ||
|
|
||
| // Day after DST (March 30, 2025) - CEST (UTC+2) | ||
| vi.setSystemTime(new Date('2025-03-30T12:00:00Z')) | ||
| timer.setSun('post-dst-berlin', params) | ||
| const postTime = timer.getSunNextExecuteTime('post-dst-berlin') | ||
|
|
||
| expect(preTime).not.toBeNull() | ||
| expect(postTime).not.toBeNull() | ||
|
|
||
| // Convert to local time strings for comparison | ||
| const preDate = new Date(preTime!) | ||
| const postDate = new Date(postTime!) | ||
| const preLocalHour = preDate.getHours() | ||
| const postLocalHour = postDate.getHours() | ||
|
|
||
| // Local time should be approximately the same | ||
| const hourDifference = Math.abs(postLocalHour - preLocalHour) | ||
| expect(hourDifference).toBeLessThan(3) | ||
| } finally { | ||
| vi.useRealTimers() | ||
| } |
There was a problem hiding this comment.
The DST boundary tests on lines 1232-1342 use getHours() which returns the hour in the system's local timezone, not the timezone of the location being tested (New York, Berlin, Sydney). This means the test results will vary depending on where the test is run.
For example, if the test runs on a machine in UTC timezone, preDate.getHours() will return the UTC hour, not the New York local hour. This makes the test unreliable and potentially incorrect.
To properly test DST behavior, you should either:
- Use
toLocaleString()with the appropriate timezone (but fix the parsing issue noted in another comment) - Convert the UTC timestamp to the expected local time mathematically by applying the timezone offset
- Use a library like dayjs with timezone support to convert to the target timezone
Example:
const preLocalHour = dayjs(preTime).tz('America/New_York').hour()
const postLocalHour = dayjs(postTime).tz('America/New_York').hour()| // Get the UTC date that corresponds to this local date | ||
| const localDateAtMidnight = new Date(now.getFullYear(), now.getMonth(), now.getDate()) | ||
| const utcYear = localDateAtMidnight.getUTCFullYear() | ||
| const utcMonth = localDateAtMidnight.getUTCMonth() | ||
| const utcDate = localDateAtMidnight.getUTCDate() |
There was a problem hiding this comment.
The logic in lines 346-353 may produce incorrect results when the local date crosses a UTC date boundary. For example, if the local time is 11 PM on June 20th but UTC is already June 21st, localDateAtMidnight will be June 20th in local time, but when you extract UTC components from it, you'll get June 21st. This means the sun calculation will be done for June 21st when it should be for June 20th.
Consider using the UTC components directly from now instead:
const utcYear = now.getUTCFullYear()
const utcMonth = now.getUTCMonth()
const utcDate = now.getUTCDate()This ensures the UTC date components match the date that was used for the day-of-year calculation on line 276.
| // Get the UTC date that corresponds to this local date | |
| const localDateAtMidnight = new Date(now.getFullYear(), now.getMonth(), now.getDate()) | |
| const utcYear = localDateAtMidnight.getUTCFullYear() | |
| const utcMonth = localDateAtMidnight.getUTCMonth() | |
| const utcDate = localDateAtMidnight.getUTCDate() | |
| // Get the UTC date components directly from 'now' | |
| const utcYear = now.getUTCFullYear() | |
| const utcMonth = now.getUTCMonth() | |
| const utcDate = now.getUTCDate() |
| const jan1 = dayjs(new Date(now.getFullYear(), 0, 1)) | ||
| const today = dayjs(new Date(now.getFullYear(), now.getMonth(), now.getDate())) |
There was a problem hiding this comment.
There's a potential issue with the day-of-year calculation using dayjs. On lines 274-276, you're creating new Date objects with local timezone for both dates before passing them to dayjs. This means if DST occurs on January 1st or the current date (unlikely but theoretically possible in some locales), or if the system timezone differs from the user's intent, this could still introduce timezone-related issues.
Consider using dayjs with UTC mode consistently:
const jan1 = dayjs.utc(new Date(Date.UTC(now.getUTCFullYear(), 0, 1)))
const today = dayjs.utc(new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate())))
const day = today.diff(jan1, 'day')This would ensure the day-of-year calculation is purely in UTC, completely eliminating any DST or timezone concerns.
| const jan1 = dayjs(new Date(now.getFullYear(), 0, 1)) | |
| const today = dayjs(new Date(now.getFullYear(), now.getMonth(), now.getDate())) | |
| const jan1 = dayjs.utc(new Date(Date.UTC(now.getUTCFullYear(), 0, 1))) | |
| const today = dayjs.utc(new Date(Date.UTC(now.getUTCFullYear(), now.getUTCMonth(), now.getUTCDate()))) |
| const preLocalHour = preDate.toLocaleString('en-US', { | ||
| timeZone: 'America/New_York', | ||
| hour: '2-digit', | ||
| }) | ||
| const postLocalHour = postDate.toLocaleString('en-US', { | ||
| timeZone: 'America/New_York', | ||
| hour: '2-digit', | ||
| }) | ||
|
|
||
| // Wall-clock times should be similar (within 1-2 minutes) | ||
| // This tests that the DST fix is working | ||
| const minDiff = Math.abs(parseInt(postLocalHour.split(':')[0]) - parseInt(preLocalHour.split(':')[0])) |
There was a problem hiding this comment.
The string parsing logic on line 3287 has a potential issue. The toLocaleString with hour: '2-digit' option returns just the hour (e.g., "06" or "18"), not a time string with colons. Therefore, calling .split(':')[0] will return the entire string, not just the hour part.
The correct approach would be to either:
- Use
toLocaleStringwith more options to get the full time, or - Directly use
getHours()method on the Date object to get the local hour
For example:
const preLocalHour = preDate.toLocaleString('en-US', {
timeZone: 'America/New_York',
hour: 'numeric',
hour12: false
})
const postLocalHour = postDate.toLocaleString('en-US', {
timeZone: 'America/New_York',
hour: 'numeric',
hour12: false
})
const minDiff = Math.abs(parseInt(postLocalHour) - parseInt(preLocalHour))This test may pass by accident but is logically incorrect.
| const preLocalHour = preDate.toLocaleString('en-US', { | |
| timeZone: 'America/New_York', | |
| hour: '2-digit', | |
| }) | |
| const postLocalHour = postDate.toLocaleString('en-US', { | |
| timeZone: 'America/New_York', | |
| hour: '2-digit', | |
| }) | |
| // Wall-clock times should be similar (within 1-2 minutes) | |
| // This tests that the DST fix is working | |
| const minDiff = Math.abs(parseInt(postLocalHour.split(':')[0]) - parseInt(preLocalHour.split(':')[0])) | |
| const preLocalHour = dayjs(preDate).tz('America/New_York').hour() | |
| const postLocalHour = dayjs(postDate).tz('America/New_York').hour() | |
| // Wall-clock times should be similar (within 1-2 minutes) | |
| // This tests that the DST fix is working | |
| const minDiff = Math.abs(postLocalHour - preLocalHour) |
| const minutesDiff = Math.abs(afterDate.getTime() - beforeDate.getTime()) / 60000 | ||
|
|
There was a problem hiding this comment.
Unused variable minutesDiff.
| const minutesDiff = Math.abs(afterDate.getTime() - beforeDate.getTime()) / 60000 |
| const minutesDiff = Math.abs(afterDate.getTime() - beforeDate.getTime()) / 60000 | ||
|
|
There was a problem hiding this comment.
Unused variable minutesDiff.
| const minutesDiff = Math.abs(afterDate.getTime() - beforeDate.getTime()) / 60000 |
| expect(time).not.toBeNull() | ||
| const date = new Date(time!) | ||
| const hours = date.getHours() | ||
| const minutes = date.getMinutes() |
There was a problem hiding this comment.
Unused variable minutes.
| const minutes = date.getMinutes() |
|
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. WalkthroughModified sunrise/sunset calculation in the Timer event handler to use UTC-based day-of-year logic instead of DST/timezone-dependent calculations, eliminating offset drift. Reworked sun event time construction to compute UTC timestamps, precompute nextExecute values, and introduced a new public helper method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (3)
Comment |
|
@darinpope There are a bunch of comments here that I think need resolving before this can progress. |
4 participants
The original implementation of sunrise/sunset event timing had a critical DST bug where event times would jump 1+ hours across DST transitions (issue #3737). The root cause was using getTimezoneOffset() in day-of-year calculations, which changes during DST transitions.
Key Changes
DST Fix Implementation (Timer.ts)
Testing
Debug Logging
New Test Categories
In addition to automated tests, also tested interactively with numerous lat/long combinations.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.