fix: Statistics summary timezone alignment for negative UTC users #697
Conversation
Fixed discrepancy between carb data displayed in main graph vs summary by ensuring consistent day boundary calculations between chart data and summary calculations. Key changes: - Simplified visibleDateRange() to use same startOfDay() logic as meal data grouping - Removed complex time-based alignment that caused issues for negative UTC timezones - Added day boundary alignment in calculateAveragesForDateRange() - Ensures both chart display and summary use identical date boundaries Fixes nightscout#687
Extended the timezone boundary fix to TDD and Bolus statistics which had the same date alignment issue as meal statistics. All summary calculations now use consistent startOfDay() logic matching the chart data grouping. - Fixed calculateTDDAveragesForDateRange() in TDDSetup.swift - Fixed calculateBolusAveragesForDateRange() and calculateBolusTotalsForDateRange() in BolusStatsSetup.swift - Ensures all statistics summaries work correctly for users in negative UTC timezones Related to nightscout#687
Sjoerd-Bo3
requested review from
AndreasStokholm,
MikePlante1,
bjornoleh,
dnzxy,
marv-out and
t1dude
as code owners
kingst
left a comment
kingst
left a comment
There was a problem hiding this comment.
Tested it in my simulator and it appears to be working. I left one comment, but there are a few -1 for dates can you double check to make sure that the aligning logic isn't dropping a day at the end?
- Replace sortedEvents.first\! with safe guard statement - Use TimeInterval.hours(24) for clearer date arithmetic - Addresses kingst's feedback about potential crashes and timezone handling This ensures the app won't crash with empty event arrays and makes the date calculation logic more explicit and safer.
- Test -1 second logic across full UTC offset spectrum (-11 to +14) - Verify empty events are handled without forced unwrap crashes - Test DST transitions maintain correct day boundaries - Ensure multiple consecutive days work correctly Tests specifically verify kingst's concern that the -1 second adjustment doesn't accidentally drop days with negative UTC offsets. Signed-off-by: Sjoerd Bozon <[email protected]>
|
@kingst, I've addressed your feedback about the forced unwraps and the concern about the -1 logic potentially dropping days. Thanks for catching that! Changes Made
Testing Your ConcernI added comprehensive tests specifically for your worry about the -1 second adjustment. The test testMinusOneLogicAcrossAllTimezones() verifies that:
For example, in Hawaii (UTC-10), when it's 11:30 PM on Jan 15, it's already 9:30 AM Jan 16 in UTC. The tests confirm that our calculation still correctly produces Jan 15 00:00:00 to Jan 15 23:59:59, not accidentally dropping back to Also tested:
The fix is split into two commits:
Let me know if you'd like me to adjust anything else! |
kingst
left a comment
kingst
left a comment
There was a problem hiding this comment.
Looks good! I'm still seeing a forced unwrap on line 44 of Trio/Sources/Modules/Stat/View/StatChartUtils.swift but once you fix that I can approve
Address kingst's code review feedback by replacing forced unwrap with nil coalescing operator on line 44, providing a safe fallback to endOfDay if the date calculation fails.
@kingst Fixed |
MikePlante1
left a comment
MikePlante1
left a comment
There was a problem hiding this comment.
The ending of visible date is incorrect with this PR applied. It should say Jul 2 - Jul 8, but instead it says Jul 2 - Jul 9. I checked the Week charts for TDD, Meals, Bolus Distribution, Percentile (by day), and Distribution (by day), and they all showed Jul 2 - Jul 9.
| start | end |
|---|---|
 |
 |
|
@Sjoerd-Bo3 can confirm @MikePlante1 comment. It is less critical than what you fixed, but most probably can be remedied by setting that end date -1? |
|
Status here? |
|
3 months later, status here? |
|
I have been using this for the 3 months, the issue with d+1 being shown in the the time intervall for week or month etc.. Mike mentions is still there. But it’s the lesser evil compared to what was being fixed. |
|
So this PR isn’t done cause we don’t fix one thing to break another. Please fix the date offset, then this can move up. |
5 participants
Summary
Description
This PR resolves issue #687 where users (particularly in negative UTC timezones like UTC-7) experienced discrepancies between statistics chart data and summary calculations. The main symptom was charts correctly showing 75g of carbs while the summary displayed 0g.
Root Cause
The issue stemmed from inconsistent date boundary calculations:
StatChartUtils.visibleDateRange()calendar.startOfDay()grouping during data collectionThis created misaligned date ranges that failed to match cached data, especially problematic for users in negative UTC offsets.
Solution
Standardized all date boundary logic to use consistent
calendar.startOfDay()approach:Test plan
Fixes #687