-
-
Notifications
You must be signed in to change notification settings - Fork 13
fix(orm): correcting timezone for Date returned in nested queries #450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. WalkthroughChanged Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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. Comment |
|
Claude finished @ymc9's task —— View job PR Review AnalysisReviewing the timezone fix for nested queries in PostgreSQL dialect. Todo List
Code Review SummaryThis PR addresses a timezone issue in PostgreSQL's Analysis of ChangesFile: The change modifies string date parsing to ensure UTC interpretation: - return new Date(value);
+ // force interpret as UTC
+ return new Date(value.endsWith('Z') ? value : `${value}Z`);Review Findings✅ Strengths
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/orm/src/client/crud/dialects/postgresql.ts (1)
107-111: Make UTC coercion robust to existing timezone offsets and clarify assumptionsThe new logic correctly forces bare timestamp strings to be interpreted as UTC, which should fix the nested JSON case. However:
- If
transformOutputDateever receives a string with an explicit numeric offset (e.g.2025-01-01T12:00:00+02:00),endsWith('Z')is false and you’ll appendZ, producing something like...+02:00Z, which is invalid fornew Date.- You’re also not trimming potential whitespace, so a value like
'2025-01-01T00:00:00Z 'would not matchendsWith('Z').If you expect only bare timestamps or
...Zright now, this is fine, but it’s easy to future‑proof with a small tweak:- private transformOutputDate(value: unknown) { - if (typeof value === 'string') { - // force interpret as UTC - return new Date(value.endsWith('Z') ? value : `${value}Z`); + private transformOutputDate(value: unknown) { + if (typeof value === 'string') { + const trimmed = value.trim(); + // If there is already a timezone designator (Z or ±HH:MM), use as-is; + // otherwise, treat the timestamp as UTC by appending 'Z'. + const hasTimezone = + /[zZ]$/.test(trimmed) || /[+-]\d{2}:\d{2}$/.test(trimmed); + const dateString = hasTimezone ? trimmed : `${trimmed}Z`; + return new Date(dateString);You might also want to add tests that cover:
- JSON‑returned
timestampwithout timezone (no suffix) → interpreted as UTC.- JSON‑returned values with
Zand with+HH:MM/-HH:MMoffsets (if those can occur) → parsed without modification.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/crud/dialects/postgresql.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a timezone handling issue for DateTime fields returned in nested queries when using the PostgreSQL dialect. The issue occurs because PostgreSQL's jsonb_build_object function serializes timestamptz values as ISO 8601 strings in UTC format but without the 'Z' timezone indicator, causing JavaScript's Date constructor to incorrectly interpret them as local time.
Key Changes:
- Modified
transformOutputDateto append 'Z' suffix to datetime strings that don't already have one, ensuring they're interpreted as UTC - Added comment explaining the rationale for forcing UTC interpretation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <[email protected]>
|
Claude finished @ymc9's task —— View job Code Review SummaryThis PR addresses a timezone issue in PostgreSQL's Analysis of ChangesFile: The change modifies string date parsing to ensure UTC interpretation: - return new Date(value);
+ // PostgreSQL's jsonb_build_object serializes timestamptz as ISO 8601 strings
+ // in UTC but without the 'Z' suffix (e.g., "2023-01-01T12:00:00.123456").
+ // We add 'Z' to explicitly mark them as UTC for correct Date object creation.
+ return new Date(value.endsWith('Z') ? value : `${value}Z`);Review Findings✅ Strengths
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.