Skip to content
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.

Sign up for GitHub

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

fix: local fromNow format timezone-aware #219

Merged
merged 2 commits into from
May 6, 2024
Merged

fix: local fromNow format timezone-aware #219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d2bf27
fix: local fromNow format timezone-aware
duyet May 6, 2024
2a2de06
chore: update components/data-table/cells/related-time-format.tsx
duyet May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions app/api/timezone/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { NextResponse } from 'next/server'

import { fetchData } from '@/lib/clickhouse'

export async function GET() {
try {
const resp = await fetchData<{ tz: string }[]>({
query: 'SELECT timezone() as tz',
})
return NextResponse.json({
tz: resp[0].tz,
})
} catch {
return NextResponse.json({}, { status: 500 })
}
}
22 changes: 0 additions & 22 deletions app/api/version/route.ts
View file
Open in desktop

This file was deleted.

11 changes: 4 additions & 7 deletions components/data-table/cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
ColumnFormat,
ColumnFormatOptions,
} from '@/components/data-table/column-defs'
import dayjs from '@/lib/dayjs'
import { formatReadableQuantity } from '@/lib/format-readable'

import { ActionFormat } from './cells/action-format'
Expand All @@ -15,7 +14,9 @@ import { BadgeFormat } from './cells/badge-format'
import { BooleanFormat } from './cells/boolean-format'
import { CodeToggleFormat } from './cells/code-toggle-format'
import { ColoredBadgeFormat } from './cells/colored-badge-format'
import { DurationFormat } from './cells/duration-format'
import { LinkFormat } from './cells/link-format'
import { RelatedTimeFormat } from './cells/related-time-format'

export const formatCell = <TData extends RowData, TValue>(
table: Table<TData>,
Expand Down Expand Up @@ -52,14 +53,10 @@ export const formatCell = <TData extends RowData, TValue>(
return <CodeToggleFormat row={row} value={value} />

case ColumnFormat.RelatedTime:
let fromNow = dayjs(value as string).fromNow()
return <span title={value as string}>{fromNow}</span>
return <RelatedTimeFormat value={value} />

case ColumnFormat.Duration:
let humanized = dayjs
.duration({ seconds: parseFloat(value as string) })
.humanize()
return <span title={value as string}>{humanized}</span>
return <DurationFormat value={value} />

case ColumnFormat.Boolean:
return <BooleanFormat value={value} />
Expand Down
13 changes: 13 additions & 0 deletions components/data-table/cells/duration-format.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import dayjs from '@/lib/dayjs'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


interface DurationFormatProps {
value: any
}

export async function DurationFormat({ value }: DurationFormatProps) {
let humanized = dayjs
.duration({ seconds: parseFloat(value as string) })
.humanize()

return <span title={value as string}>{humanized}</span>
}
28 changes: 28 additions & 0 deletions components/data-table/cells/related-time-format.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import dayjs from '@/lib/dayjs'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)


interface RelatedTimeFormatProps {
value: any
}

let tz: string = ""

export async function RelatedTimeFormat({ value }: RelatedTimeFormatProps) {
let fromNow
try {
if (!tz) {
// Getting timezone from server
tz = await fetch('/api/timezone')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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);
}

.then((res) => res.json())
.then((data) => data.tz)
console.log('Server timezone:', tz)
}

let parsed = dayjs.tz(value as string, tz)
fromNow = parsed.fromNow()
} catch (e) {
console.error('Error parsing time:', e)
fromNow = dayjs(value as string).fromNow()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will not using timezone here

}

return <span title={value as string}>{fromNow}</span>
}
4 changes: 4 additions & 0 deletions lib/dayjs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
import timezone from 'dayjs/plugin/timezone'
import timezone from 'dayjs/plugin/timezone'
dayjs.extend(timezone)

import utc from 'dayjs/plugin/utc'

import 'dayjs/locale/en'

dayjs.extend(duration)
dayjs.extend(utc)
dayjs.extend(timezone)
dayjs.extend(relativeTime)

export default dayjs