fix: resolve timezone-dependent date calculation errors#399
fix: resolve timezone-dependent date calculation errors#399praaatap wants to merge 1 commit intofossasia:mainfrom
Conversation
Reviewer's GuideReplaces UTC-based date string generation with a new local-time-based helper and applies it across main popup and scrum helper logic to fix off-by-one date errors in different timezones, plus a small refactor in the repo filter callback. Sequence diagram for local date range calculation during report generationsequenceDiagram
actor User
participant PopupUI
participant MainScript
participant ScrumHelper
participant GitHubAPI
User->>PopupUI: Click generateReport
PopupUI->>MainScript: requestDateRange()
alt yesterdayContribution enabled
MainScript->>MainScript: today = new Date()
MainScript->>MainScript: yesterday = new Date(today.getTime() - 24h)
MainScript->>MainScript: startDate = getLocalDateString(yesterday)
MainScript->>MainScript: endDate = getLocalDateString(today)
else custom or default range
MainScript->>MainScript: compute startDate and endDate using getLocalDateString
end
MainScript->>ScrumHelper: generateReport(startDate, endDate)
ScrumHelper->>ScrumHelper: startDateTime = new Date(startDate + T00:00:00Z)
ScrumHelper->>ScrumHelper: endDateTime = new Date(endDate + T23:59:59Z)
ScrumHelper->>GitHubAPI: fetchCommits(startDate, endDate)
GitHubAPI-->>ScrumHelper: commits
ScrumHelper-->>PopupUI: reportData
PopupUI-->>User: Show correct yesterday/today report (local timezone)
Class diagram for local date helper and consumersclassDiagram
class MainScript {
+getLocalDateString(date Date) string
+getToday() string
+getYesterday() string
}
class PopupScript {
+getToday() string
+getYesterday() string
}
class ScrumHelperScript {
+getToday() string
+getYesterday() string
+computeCacheRange(yesterdayContribution boolean, startingDate string, endingDate string) void
+computeCommitRange(yesterdayContribution boolean, startingDate string, endingDate string) void
+computeActivityRange(yesterdayContribution boolean, startingDate string, endingDate string) void
+buildDateFilters(yesterdayContribution boolean, startingDate string, endingDate string) void
+fetchUserRepositories(username string, token string, org string) void
}
MainScript <.. PopupScript : uses getLocalDateString
MainScript <.. ScrumHelperScript : uses getLocalDateString
class RepoFilter {
+filterAvailableRepos(availableRepos Array, selectedRepos Array, query string) Array
}
PopupScript <.. RepoFilter : calls filterAvailableRepos
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
javascript
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider moving
getLocalDateStringinto a shared utilities module (or clearly documented global) and importing/attaching it consistently, rather than defining it inmain.jsand relying on it being available in other scripts. - Where you now use
getLocalDateString(...)+ 'T00:00:00Z'/'T23:59:59Z', double-check that treating a local calendar date as UTC midnight is intentional; if you truly want local-day ranges, you may want to constructDateobjects in local time and only convert to UTC when sending to APIs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving `getLocalDateString` into a shared utilities module (or clearly documented global) and importing/attaching it consistently, rather than defining it in `main.js` and relying on it being available in other scripts.
- Where you now use `getLocalDateString(...)+ 'T00:00:00Z'` / `'T23:59:59Z'`, double-check that treating a local calendar date as UTC midnight is intentional; if you truly want local-day ranges, you may want to construct `Date` objects in local time and only convert to UTC when sending to APIs.
## Individual Comments
### Comment 1
<location path="src/scripts/scrumHelper.js" line_range="1535-1536" />
<code_context>
const yesterday = new Date(today.getTime() - 24 * 60 * 60 * 1000);
- startDateFilter = new Date(yesterday.toISOString().split('T')[0] + 'T00:00:00Z');
- endDateFilter = new Date(today.toISOString().split('T')[0] + 'T23:59:59Z'); // Use yesterday for start and today for end
+ startDateFilter = new Date(getLocalDateString(yesterday) + 'T00:00:00Z');
+ endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); // Use yesterday for start and today for end
} else if (startingDate && endingDate) {
startDateFilter = new Date(startingDate + 'T00:00:00Z');
</code_context>
<issue_to_address>
**issue (bug_risk):** Using a local calendar date with a hardcoded `Z` suffix can shift the effective range by a day depending on the user’s timezone.
`toISOString().split('T')[0]` gave you a UTC date, so appending `'T00:00:00Z'` / `'T23:59:59Z'` kept everything in UTC. `getLocalDateString` instead uses the local calendar date, but you still append `'Z'`, which tells the runtime to interpret that local date as UTC. For users far from UTC, this shifts the actual time window compared to their local day and any UTC-based logic elsewhere.
To avoid this, either:
- keep everything local (e.g. `new Date(getLocalDateString(...) + 'T00:00:00')` with no `Z`), or
- keep everything UTC (continue using `toISOString().split('T')[0]`).
Please pick one (local vs UTC) and apply it consistently wherever these date strings are turned into `Date` objects.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| startDateFilter = new Date(getLocalDateString(yesterday) + 'T00:00:00Z'); | ||
| endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); // Use yesterday for start and today for end |
There was a problem hiding this comment.
issue (bug_risk): Using a local calendar date with a hardcoded Z suffix can shift the effective range by a day depending on the user’s timezone.
toISOString().split('T')[0] gave you a UTC date, so appending 'T00:00:00Z' / 'T23:59:59Z' kept everything in UTC. getLocalDateString instead uses the local calendar date, but you still append 'Z', which tells the runtime to interpret that local date as UTC. For users far from UTC, this shifts the actual time window compared to their local day and any UTC-based logic elsewhere.
To avoid this, either:
- keep everything local (e.g.
new Date(getLocalDateString(...) + 'T00:00:00')with noZ), or - keep everything UTC (continue using
toISOString().split('T')[0]).
Please pick one (local vs UTC) and apply it consistently wherever these date strings are turned into Date objects.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #398 by switching date formatting from UTC-based toISOString().split('T')[0] to a local-date formatter so “today/yesterday” calculations don’t shift by timezone.
Changes:
- Added a
getLocalDateString(date)formatter and updated “today/yesterday” helpers to use it. - Replaced multiple UTC date-string usages in
scrumHelper.jsto avoid off-by-one local dates. - Minor refactor of repo filtering callback formatting in
popup.js.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/scripts/scrumHelper.js |
Replaces UTC date-string formatting with getLocalDateString(...) across several date-range calculations and filters. |
src/scripts/popup.js |
Updates “today/yesterday” to use getLocalDateString and reformats repo filter callback. |
src/scripts/main.js |
Introduces getLocalDateString(date) and updates “today/yesterday” to use it. |
Comments suppressed due to low confidence (1)
src/scripts/scrumHelper.js:1211
- These boundaries are still constructed as UTC times (
...T00:00:00Z/...T23:59:59Z). After switchingstartDate/endDateto local-date strings, appendingZmakes the range UTC midnight, not the user's local midnight, so filtering can still be off by the timezone offset. Build the range in local time (e.g., usingnew Date(year, monthIndex, day, 0,0,0,0)/ end-of-day) instead of parsing aZ-suffixed string.
const startDateTime = new Date(startDate + 'T00:00:00Z');
const endDateTime = new Date(endDate + 'T23:59:59Z');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getYesterday() { | ||
| const today = new Date(); | ||
| const yesterday = new Date(today); | ||
| yesterday.setDate(today.getDate() - 1); | ||
| return yesterday.toISOString().split('T')[0]; | ||
| return getLocalDateString(yesterday); | ||
| } | ||
| function getToday() { | ||
| const today = new Date(); | ||
| return today.toISOString().split('T')[0]; | ||
| return getLocalDateString(today); |
There was a problem hiding this comment.
getLocalDateString is not defined anywhere in scrumHelper.js, and main.js is not part of the content_scripts list in manifest.json (only jquery, emailClientAdapter, gitlabHelper, and scrumHelper are injected). This will throw a ReferenceError when the content script runs on mail pages. Define/import getLocalDateString within scrumHelper.js (or extract it into a shared utility file that is included by both the popup page and the content script via manifest.json).
| startDateFilter = new Date(getLocalDateString(yesterday) + 'T00:00:00Z'); | ||
| endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); // Use yesterday for start and today for end | ||
| } else if (startingDate && endingDate) { | ||
| startDateFilter = new Date(startingDate + 'T00:00:00Z'); | ||
| endDateFilter = new Date(endingDate + 'T23:59:59Z'); | ||
| } else { | ||
| // Default to last 7 days if no date range is set | ||
| const today = new Date(); | ||
| const lastWeek = new Date(today.getFullYear(), today.getMonth(), today.getDate() - 7); | ||
| startDateFilter = new Date(lastWeek.toISOString().split('T')[0] + 'T00:00:00Z'); | ||
| endDateFilter = new Date(today.toISOString().split('T')[0] + 'T23:59:59Z'); | ||
| startDateFilter = new Date(getLocalDateString(lastWeek) + 'T00:00:00Z'); | ||
| endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); | ||
| } |
There was a problem hiding this comment.
Same issue here: new Date(getLocalDateString(...) + 'T00:00:00Z') creates a UTC-based day window, not a local-time window. Since the goal is to filter by the user's local day, compute startDateFilter/endDateFilter using local time constructors (or at least omit the trailing Z) so comparisons against created_at/updated_at align with local-day boundaries.
|
Thanks @praaatap for your contribution. Please address copilots' and sourcey's concerns! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/scripts/scrumHelper.js:1210
- After fixing the date string generation to use local dates, these Date objects are constructed incorrectly. Appending 'T00:00:00Z' treats the local date string as if it's in UTC timezone, which reintroduces timezone issues.
For example, if a user in UTC+9 on January 15 wants "yesterday" data:
getLocalDateString(yesterday)correctly returns "2024-01-14"- But
new Date("2024-01-14T00:00:00Z")creates a Date representing January 14 midnight UTC - In the user's timezone (UTC+9), this is actually January 14 at 9:00 AM JST, not midnight JST
To properly represent the user's local midnight, remove the 'Z' suffix (which forces UTC interpretation) or use a different approach like creating Date objects directly from the local date variables with time set to midnight.
const startDateTime = new Date(startDate + 'T00:00:00Z');
const endDateTime = new Date(endDate + 'T23:59:59Z');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const yesterday = new Date(today); | ||
| yesterday.setDate(today.getDate() - 1); | ||
| return yesterday.toISOString().split('T')[0]; | ||
| return getLocalDateString(yesterday); |
There was a problem hiding this comment.
The function getLocalDateString is not defined in this file or in any file loaded in the content script context. According to manifest.json, the content scripts only load jquery-3.2.1.min.js, emailClientAdapter.js, gitlabHelper.js, and scrumHelper.js - but main.js (where getLocalDateString is defined) is not included. This will cause a ReferenceError when scrumHelper.js runs as a content script on email client pages.
To fix this issue, you need to either:
- Add
scripts/main.jsto the content_scripts array inmanifest.jsonbeforescrumHelper.js, OR - Define
getLocalDateStringdirectly inscrumHelper.js, OR - Create a shared utility file that's loaded in both the popup and content script contexts
The same issue affects all other calls to getLocalDateString in this file (lines 369, 480, 481, 489, 490, 713, 714, 722, 723, 1196, 1197, 1205, 1206, 1426, 1427, 1435, 1436, 1535, 1536, 1544, 1545, 1916, 1917, 1924, 1925, 1934, 1935).
| startDateFilter = new Date(getLocalDateString(yesterday) + 'T00:00:00Z'); | ||
| endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); // Use yesterday for start and today for end |
There was a problem hiding this comment.
Same timezone issue as lines 1209-1210. After fixing the date string generation to use local dates, appending 'T00:00:00Z' treats the local date string as UTC timezone, which reintroduces timezone problems. Remove the 'Z' suffix from both date string constructions to properly represent the user's local timezone boundaries.
| startDateFilter = new Date(getLocalDateString(lastWeek) + 'T00:00:00Z'); | ||
| endDateFilter = new Date(getLocalDateString(today) + 'T23:59:59Z'); |
There was a problem hiding this comment.
Same timezone issue. The 'Z' suffix in the date string construction forces UTC interpretation, undermining the fix to use local dates. Remove the 'Z' suffix to properly represent local timezone boundaries.
| startDateFilter = new Date(startingDate + 'T00:00:00Z'); | ||
| endDateFilter = new Date(endingDate + 'T23:59:59Z'); |
There was a problem hiding this comment.
These lines have the same timezone issue. When startingDate and endingDate come from user input in the date picker, they are already local dates in YYYY-MM-DD format. Appending 'T00:00:00Z' treats them as UTC dates, which causes incorrect filtering for users not in UTC timezone. Remove the 'Z' suffix from both lines to properly interpret these as local timezone boundaries.
| const startDate = thirtyDaysAgo.toISOString().split('T')[0]; | ||
| const endDate = today.toISOString().split('T')[0]; | ||
| const startDate = getLocalDateString(thirtyDaysAgo); | ||
| const endDate = getLocalDateString(today); |
There was a problem hiding this comment.
The startDate and endDate variables defined in this catch block are scoped locally and don't update the dateRange variable that was set before the catch block. This means if the date range determination fails, the function will continue using an empty dateRange string rather than falling back to the 30-day range. This is a pre-existing bug, but while modifying these lines for the timezone fix, consider also fixing this by either: (1) removing the const and declaring these variables at the function level, or (2) setting dateRange = \+created:${startDate}..${endDate}`` inside the catch block.
| const endDate = getLocalDateString(today); | |
| const endDate = getLocalDateString(today); | |
| dateRange = `+created:${startDate}..${endDate}`; | |
| console.log(`Falling back to last 30 days for repo search: ${startDate} to ${endDate}`); |
|
Thanks @praaatap for your contribution. Please resolve the merge conflicts and address the Copilot review comments. Also, add a short video or before/after screenshots of the feature implementation (whichever is applicable) to help with review. This PR has been inactive for a while and is scheduled to be closed soon, so please push the required updates at the earliest. |
📌 Fixes
#398
📝 Summary of Changes
toISOString().split('T')[0]with a newgetLocalDateStringutility function to resolve off-by-one timezone date calculation errors.main.js,popup.js, andscrumHelper.js.📸 Screenshots / Demo (if UI-related)
N/A - Logic fix
✅ Checklist
👀 Reviewer Notes
This fixes an issue where users in timezones like UTC+9 might see the previous day's data when creating a report early in their morning because
toISOString()returns the UTC date instead of the user's local date.Summary by Sourcery
Fix timezone-dependent date calculations by standardizing on a local date utility and updating callers to use it.
Bug Fixes:
Enhancements: