fix: local fromNow format timezone-aware #219
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| try { | ||
| if (!tz) { | ||
| // Getting timezone from server | ||
| tz = await fetch('/api/timezone') |
There was a problem hiding this comment.
suggestion (code_refinement): Consider error handling for the fetch request to improve reliability.
Adding error handling for the fetch request can prevent the application from breaking if the API call fails.
| tz = await fetch('/api/timezone') | |
| try { | |
| const response = await fetch('/api/timezone'); | |
| if (!response.ok) { | |
| throw new Error('Failed to fetch timezone'); | |
| } | |
| const data = await response.json(); | |
| tz = data.tz; | |
| } catch (error) { | |
| console.error('Error fetching timezone:', error); | |
| } |
| fromNow = parsed.fromNow() | ||
| } catch (e) { | ||
| console.error('Error parsing time:', e) | ||
| fromNow = dayjs(value as string).fromNow() |
There was a problem hiding this comment.
issue (bug_risk): Fallback logic in catch block may not consider timezone.
The fallback logic uses the default timezone instead of the server timezone, which might not be intended.
There was a problem hiding this comment.
Will not using timezone here
| @@ -0,0 +1,28 @@ | |||
| import dayjs from '@/lib/dayjs' | |||
There was a problem hiding this comment.
suggestion (testing): Missing test for RelatedTimeFormat component
The new RelatedTimeFormat component handles timezone-aware formatting. It's crucial to ensure this component behaves as expected under various conditions, including edge cases like invalid date formats or null values. Please add unit tests to verify the functionality and error handling.
| import dayjs from '@/lib/dayjs' | |
| import dayjs from '@/lib/dayjs' | |
| import timezone from 'dayjs/plugin/timezone' | |
| import utc from 'dayjs/plugin/utc' | |
| dayjs.extend(timezone) | |
| dayjs.extend(utc) |
| @@ -0,0 +1,13 @@ | |||
| import dayjs from '@/lib/dayjs' | |||
There was a problem hiding this comment.
suggestion (testing): Missing test for DurationFormat component
The DurationFormat component has been introduced to handle human-readable duration formatting. Testing this component is essential to ensure it accurately converts various duration inputs into the correct format. Please add tests to cover typical, boundary, and erroneous input scenarios.
| @@ -1,10 +1,14 @@ | |||
| import dayjs from 'dayjs' | |||
| import duration from 'dayjs/plugin/duration' | |||
| import relativeTime from 'dayjs/plugin/relativeTime' | |||
| import timezone from 'dayjs/plugin/timezone' | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for timezone and UTC configurations in dayjs
With the addition of timezone and UTC plugins to the dayjs configuration, it's important to verify that these settings are correctly applied across the application. This could involve checking the correct timezone adjustments and UTC handling in date/time calculations.
| import timezone from 'dayjs/plugin/timezone' | |
| import timezone from 'dayjs/plugin/timezone' | |
| dayjs.extend(timezone) |
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Signed-off-by: Duyet Le <5009534+duyet@users.noreply.github.com>
Fix #54