fix(core): revert incorrect time zone fixes #278
Conversation
The previous fix was still wrong. This fix makes sure that all "local" datetimes use the environment (process.env.TZ) time zone, and that offsets are not applied twice.
WalkthroughThe changes in this pull request involve modifications to several functions related to date and time handling, middleware processing, and message formatting within the codebase. Key updates include a refined approach to calculating local and UTC dates, enhancements to middleware for better message processing, and simplifications in the logic of specific utility functions. Overall, the changes aim to improve accuracy and efficiency in handling time-related operations and chat interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Middleware
participant ChatService
participant DateHandler
User->>Middleware: Send message
Middleware->>DateHandler: Get current date
DateHandler-->>Middleware: Return current date
Middleware->>ChatService: Format and send prompt
ChatService-->>Middleware: Return response
Middleware->>User: Send response
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/core/src/llm-core/prompt/format.ts (2)
Line range hint
209-217: Simplify message type instantiation using class mappingIn the
formatPresetTemplatefunction, you can enhance readability by defining a mapping of message types to their respective classes and then instantiating them accordingly:const messageTypeMap = { human: HumanMessage, ai: AIMessage, system: SystemMessage } const messageInstance = new messageTypeMap[message._getType()]({ content, additional_kwargs: message.additional_kwargs })This refactoring improves code clarity and makes it easier to manage message types in the future.
141-144: Improve code comments for clarity on timezone handlingThe comments explaining the timezone logic can be clarified to enhance understanding:
// `Date` methods like `getUTC...()`, `toUTCString()`, and `toISOString()` return UTC values. // Other methods return local time values. // `date` represents the current local date and time. // `offsetDate` adjusts for the timezone offset to synchronize local and UTC time representations.Clarifying these comments can help future developers grasp the timezone adjustments more quickly.
packages/core/src/middlewares/request_model.ts (1)
280-282: Consider locale specificity in date and time formattingWhen using
toLocaleTimeString()andgetCurrentWeekday(), the output may vary based on the server's locale settings. For a consistent user experience across different regions, consider specifying the locale explicitly or using a localization library to handle internationalization appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/llm-core/prompt/format.ts (3 hunks)
- packages/core/src/middlewares/request_model.ts (2 hunks)
- packages/core/src/utils/string.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
packages/core/src/utils/string.ts (2)
Line range hint
1-114: Consider refactoring all time-related functions for consistent time zone handlingWhile addressing the
getTimeInUTCfunction, it's worth considering a broader refactoring of all time-related functions in this file to ensure consistent and correct time zone handling across the codebase.Here are some suggestions:
For
getCurrentWeekday:export function getCurrentWeekday() { const localZone = process.env.TZ || 'local'; return DateTime.now().setZone(localZone).weekdayLong; }For
getTimeDiffFormatandgetTimeDiff:export const getTimeDiffFormat = (time1: DateTime, time2: DateTime): string => { const diff = Math.abs(time1.diff(time2, ['days', 'hours', 'minutes']).toObject()); // ... rest of the function remains similar } export const getTimeDiff = (time1: string, time2: string): string => { const localZone = process.env.TZ || 'local'; return getTimeDiffFormat( DateTime.fromISO(time1, { zone: localZone }), DateTime.fromISO(time2, { zone: localZone }) ); }These changes would ensure all time-related functions consistently use the time zone specified in
process.env.TZ, aligning with the PR objectives.To check the usage of these time-related functions across the codebase:
#!/bin/bash echo "Usage of time-related functions:" rg --type typescript "getCurrentWeekday|getTimeDiffFormat|getTimeDiff" -C 2Please review these suggestions and consider implementing a consistent approach to time zone handling across all relevant functions.
55-59:⚠️ Potential issueRefactor
getTimeInUTCfunction for accuracy and clarityThe current implementation of
getTimeInUTChas several issues:
- The function name is misleading as it's not returning a UTC time.
- The substring operation is incorrect and will return an empty string.
- It doesn't explicitly use the time zone from
process.env.TZas mentioned in the PR objectives.To address these issues, consider the following refactoring:
import { DateTime } from 'luxon'; export const getAdjustedLocalTime = (offsetHours: number): string => { const localZone = process.env.TZ || 'local'; const now = DateTime.now().setZone(localZone); const adjusted = now.plus({ hours: offsetHours }); return adjusted.toFormat('HH:mm:ss'); };This refactoring:
- Renames the function to better reflect its purpose.
- Uses Luxon for robust time zone handling.
- Explicitly uses the time zone from
process.env.TZ.- Correctly formats the time string.
To ensure the
luxonpackage is available, run:Please review this suggestion and let me know if you'd like any modifications or have any questions.
packages/core/src/llm-core/prompt/format.ts (3)
145-146:⚠️ Potential issueVerify the correctness of 'offsetDate' calculation for timezone adjustments
The computation of
offsetDatesubtractsdate.getTimezoneOffset() * Time.minutefrom the current date:const date = new Date() const offsetDate = new Date(+date - date.getTimezoneOffset() * Time.minute)Since
date.getTimezoneOffset()returns the difference between UTC time and local time in minutes (UTC - local time), subtracting it may not correctly adjust to the intended timezone. Please verify that this calculation provides the correct local date and time across different timezones.To confirm the correctness, you might consider running tests in various timezones or utilizing well-tested libraries for timezone handling.
155-157:⚠️ Potential issuePotential misunderstanding of 'getTimezoneOffset()' in 'time_UTC' function
In the
time_UTCcase, the code adds theutcOffsetto the current date:// The offset is added instead of subtracted here because `Date.getTimezoneOffset()` is negative. const offsetDate = new Date(+date + utcOffset * Time.hour) return offsetDate.toISOString().replace('T', ' ').slice(0, -5)However,
date.getTimezoneOffset()returns the offset in minutes from local time to UTC (i.e.,UTC - local time). This value is positive if the local timezone is behind UTC and negative if it's ahead. The comment mentions thatgetTimezoneOffset()is negative, which may not always be the case. Please verify the logic to ensure that time calculations are accurate for all timezones.Consider using UTC methods or reliable timezone libraries to handle these calculations more robustly.
163-163:⚠️ Potential issueInconsistent use of 'offsetDate' may lead to incorrect local times
In the
date,isotime, andisodatecases, theoffsetDateis used:case 'date': return offsetDate.toISOString().split('T')[0] case 'isotime': return offsetDate.toISOString().slice(11, 19) case 'isodate': return offsetDate.toISOString().split('T')[0]Using
offsetDate.toISOString()might not yield the correct local date and time becausetoISOString()returns the date in UTC. Given thatoffsetDatehas been adjusted with the timezone offset, applyingtoISOString()could result in double adjustments. Please review these cases to ensure that the functions return the intended local date and time.You might need to use locale-sensitive methods or format the date using options like
date.toLocaleDateString()anddate.toLocaleTimeString()for accurate local representations.Also applies to: 175-175, 177-177
packages/core/src/middlewares/request_model.ts (1)
280-285:⚠️ Potential issueVerify the argument order in
getTimeDiffFormatIn the
idle_durationcalculation,getTimeDiffFormatis called withnew Date().getTime()as the current time androom.updatedTime.getTime()as the previous time. Ensure thatgetTimeDiffFormatcomputes the difference correctly with these arguments. If the function expects arguments in the order(startTime, endTime)but receives(currentTime, previousTime), it may result in negative durations or incorrect formatting.To confirm the correct usage of
getTimeDiffFormat, you can review its implementation or compare with other usages in the codebase:#!/bin/bash # Description: Find all usages of `getTimeDiffFormat` to check argument consistency. rg --type ts 'getTimeDiffFormat\(' -A 2 -B 2
| ][date.getDay()] | ||
| case 'isotime': | ||
| return date.toISOString().slice(11, 19) | ||
| return offsetDate.toISOString().slice(11, 19) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate code in 'isodate' and 'date' cases
Both the date and isodate cases return the same value:
return offsetDate.toISOString().split('T')[0]Consider consolidating these cases to eliminate redundancy:
case 'date':
case 'isodate':
return offsetDate.toISOString().split('T')[0]This simplifies the code and maintains consistency.
Also applies to: 177-177
| return { | ||
| name: config.botName, | ||
| date: date.toLocaleString(), | ||
| date: new Date().toLocaleString(), |
There was a problem hiding this comment.
💡 Codebase verification
Time Zone Handling Review Update
No instances of toLocaleString() without explicit locale or time zone options were found. However, toLocaleTimeString() is used in request_model.ts without specifying locale or time zone, which may lead to inconsistencies across environments. Please ensure that explicit locale and time zone options are provided or consider using a reliable time zone handling library.
🔗 Analysis chain
Ensure toLocaleString() respects the desired time zone
The use of new Date().toLocaleString() may not guarantee that the time zone specified in process.env.TZ is respected. By default, it relies on the server's locale and time zone settings. To ensure consistency across different environments, consider specifying the locale and time zone explicitly using options or utilize a library like moment-timezone or date-fns-tz for reliable time zone handling.
Run the following script to identify usages of toLocaleString() without explicit locale or time zone options:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of `toLocaleString()` without locale options.
rg --type ts 'toLocaleString\(\)' -A 2 -B 2
Length of output: 826
2 participants
The previous fix was still wrong. This fix makes sure that all "local" datetimes use the environment (process.env.TZ) time zone, and that offsets are not applied twice.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation