Revert "Using UTC year/month/day in data-loader" #352
Conversation
This reverts commit 6eaf257.
|
Caution Review failedThe pull request is closed. WalkthroughThis change removes the internal helper function Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DataLoader
Caller->>DataLoader: formatDate(dateStr)
DataLoader->>DataLoader: Create Date(dateStr)
DataLoader->>DataLoader: Use local getters (getFullYear, getMonth, getDate)
DataLoader-->>Caller: Return formatted local date string
Caller->>DataLoader: formatDateCompact(dateStr)
DataLoader->>DataLoader: Create Date(dateStr)
DataLoader->>DataLoader: Use local getters (getFullYear, getMonth, getDate)
DataLoader-->>Caller: Return compact local date string
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ccusage-guide | 3123d5e | Commit Preview URL Branch Preview URL |
Jul 24 2025, 04:34 PM |
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ryoppippi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request fully reverts a previous change (#312) that introduced UTC-based date formatting in the data-loader. The primary purpose is to restore the date formatting functions (formatDate and formatDateCompact) to use local year, month, and day components, ensuring consistency with how dates were handled prior to the original change. This likely addresses an issue or an unintended side effect caused by the UTC-aware formatting.
Highlights
- Reverted Date Formatting Logic: The
formatDateandformatDateCompactfunctions insrc/data-loader.tshave been reverted to use local date components (e.g.,getFullYear(),getMonth(),getDate()) instead of attempting to interpret and use UTC components based on the input string's timezone information. This restores the previous behavior of always formatting dates based on the local timezone. - Removed Helper Function: The
formatDateInternalhelper function, which encapsulated the logic for timezone-aware date formatting, has been removed as it is no longer needed after reverting the UTC handling. - Updated Unit Tests: Corresponding unit tests for
formatDateandformatDateCompacthave been updated to reflect the change. Tests asserting explicit UTC date interpretation have been removed, and remaining tests are simplified to confirm local date formatting for various inputs, including those with UTC indicators.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request reverts a previous change, altering date formatting to rely on the local timezone. While I see this is an intentional revert, it re-introduces critical issues. The date formatting functions are now dependent on the execution environment's timezone, which can lead to incorrect data in reports. Consequently, the unit tests have become brittle and will fail in different timezones, compromising the reliability of your test suite. I've added specific comments detailing these concerns.
| export function formatDate(dateStr: string): string { | ||
| return formatDateInternal(dateStr, false); | ||
| const date = new Date(dateStr); | ||
| const year = date.getFullYear(); | ||
| const month = String(date.getMonth() + 1).padStart(2, '0'); | ||
| const day = String(date.getDate()).padStart(2, '0'); | ||
| return `${year}-${month}-${day}`; |
There was a problem hiding this comment.
Using getFullYear(), getMonth(), and getDate() unconditionally introduces a correctness issue. These methods are based on the system's local timezone. When parsing a UTC timestamp string (e.g., one ending in 'Z'), this can result in an incorrect date.
For example, in a timezone like America/Los_Angeles (UTC-8), new Date('2024-01-01T00:00:00Z') will be interpreted as 2023-12-31 16:00:00, and this function will incorrectly return '2023-12-31'.
The previous implementation correctly handled this by checking for timezone information in the date string and using UTC-based getters (getUTCFullYear, etc.) when present. This ensured consistent and correct behavior regardless of the environment's timezone.
This change could lead to data being grouped under the wrong date, affecting reports. I recommend reconsidering this revert.
| it('formats UTC timestamp to local date', () => { | ||
| // Test with UTC timestamps - results depend on local timezone | ||
| expect(formatDate('2024-01-01T00:00:00Z')).toBe('2024-01-01'); | ||
| expect(formatDate('2024-12-31T23:59:59Z')).toBe('2024-12-31'); | ||
| expect(formatDate('2024-01-01T12:00:00.000Z')).toBe('2024-01-01'); | ||
| }); |
There was a problem hiding this comment.
These tests have become brittle and non-deterministic. They will pass or fail depending on the timezone of the environment where they are run.
For example, expect(formatDate('2024-01-01T00:00:00Z')).toBe('2024-01-01'); will fail in any timezone west of UTC, because new Date('2024-01-01T00:00:00Z').getFullYear() will return 2023.
Unit tests should be independent of the execution environment to ensure they are reliable and produce consistent results for all developers and in CI/CD pipelines. The previous tests were more robust in this regard.
| it('formats UTC timestamp to local date with line break', () => { | ||
| expect(formatDateCompact('2024-01-01T00:00:00Z')).toBe('2024\n01-01'); | ||
| expect(formatDateCompact('2024-12-31T23:59:59Z')).toBe('2024\n12-31'); | ||
| expect(formatDateCompact('2024-01-01T12:00:00.000Z')).toBe('2024\n01-01'); | ||
| }); |
There was a problem hiding this comment.
Similar to the formatDate tests, these tests for formatDateCompact are now dependent on the local timezone of the machine running them, making them unreliable.
For instance, expect(formatDateCompact('2024-01-01T00:00:00Z')).toBe('2024\n01-01'); will fail in timezones where this UTC timestamp corresponds to the previous day locally.
Tests must be deterministic. Please consider making these tests timezone-agnostic to ensure they are reliable across different environments.
| export function formatDateCompact(dateStr: string): string { | ||
| return formatDateInternal(dateStr, true); | ||
| const date = new Date(dateStr); | ||
| const year = date.getFullYear(); | ||
| const month = String(date.getMonth() + 1).padStart(2, '0'); | ||
| const day = String(date.getDate()).padStart(2, '0'); | ||
| return `${year}\n${month}-${day}`; | ||
| } |
There was a problem hiding this comment.
This function has the same timezone-related correctness issue as formatDate.
Additionally, by removing the shared formatDateInternal function, the implementation is now almost identical to formatDate, leading to code duplication. The only difference is the separator used in the return string ('\n' vs '-').
This duplication makes the code harder to maintain. If the date formatting logic needs to be changed in the future, it will have to be updated in two places, increasing the risk of inconsistencies.
It would be better to extract the common logic into a shared helper function to improve maintainability.
Reverts #312
Summary by CodeRabbit
Bug Fixes
Tests