Refactor/date fns - #1069
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces selected ChangesDate localization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Date displays can be incorrect or inconsistent: some dates may appear one year old too early, server and browser rendering may disagree across time zones, and missing profile data may produce an invalid creation date. The PR should not merge until these bounded correctness issues are addressed or explicitly accepted. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Coverage Report
File CoverageNo changed files found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b4d4e3bc-162f-46da-8926-4951b398f3c8
📒 Files selected for processing (11)
app/components/device-detail/device-detail-box.tsxapp/components/device-detail/graph.tsxapp/components/map/layers/mobile/mobile-overview-layer.tsxapp/components/mydevices/dt/columns.tsxapp/components/ui/range-picker.tsxapp/lib/date.tsapp/routes/admin.devices._index.tsxapp/routes/admin.users._index.tsxapp/routes/device.$deviceId.edit.logs.tsxapp/routes/device.$deviceId.edit.transfer.tsxapp/routes/profile.$username.tsx
💤 Files with no reviewable changes (1)
- app/components/device-detail/graph.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
Gotta do the code rabbit reviews still.. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab467778-630c-4a13-a85c-18f46e3da057
📒 Files selected for processing (8)
app/components/mydevices/dt/columns.tsxapp/components/ui/range-picker.tsxapp/lib/date.tsapp/routes/admin.devices._index.tsxapp/routes/admin.users._index.tsxapp/routes/device.$deviceId.edit.logs.tsxapp/routes/device.$deviceId.edit.transfer.tsxapp/routes/profile.$username.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/routes/profile.$username.tsx (1)
163-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the creation date before formatting it.
When the loader returns
profile: null, this expression constructsnew Date('')after hydration. The formatter then receives an invalid date instead of omitting the creation date. Checkprofile?.user?.createdAtbefore callingtoLocaleDateString.Proposed fix
- {hydrated && - new Date(profile?.user?.createdAt || '').toLocaleDateString( - i18n.language, - )} + {hydrated && profile?.user?.createdAt + ? new Date(profile.user.createdAt).toLocaleDateString( + i18n.language, + ) + : null}
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cda2e5c-6c89-412c-a343-7a5efeae6ff1
📒 Files selected for processing (1)
app/routes/profile.$username.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| <div> | ||
| <p className="text-primary text-sm font-bold"> | ||
| {format(new Date(popupInfo.startTime), 'Pp')} | ||
| {dateTimeFormat.format(new Date(popupInfo.startTime))} |
There was a problem hiding this comment.
The previous format method displayed a combination of date and time, the new method requests only hours and minutes. Maybe it is fine though as we dont usually have multi-day trips?
| if (absDiffInSeconds < ONE_QUARTER_IN_S) | ||
| return r.format(-Math.round(diffInSeconds / ONE_MONTH_IN_S), 'month') | ||
| if (absDiffInSeconds < ONE_YEAR_IN_S) | ||
| return r.format(-Math.round(diffInSeconds / ONE_QUARTER_IN_S), 'quarter') |
There was a problem hiding this comment.
I dont know how this is handled in general, but quarter sounds a bit weird to me, what about having only month and year level instead?
| const { t } = useTranslation | ||
| const { t, i18n } = useTranslation | ||
| const isOwner = opts?.isOwner ?? false | ||
| const hydrated = useHydrated() |
There was a problem hiding this comment.
As getColumns is used (conditionally) in the ProfilePage component this line would violate the rules of hooks in react. This was not detected by our linting rules due to the responsible rule being turned off in oxlintrc.json at the moment (my bad 😅). As the hydration state is already present in the ProfilePage component anyway, what about passing it down to getColumns like:
export function getColumns(
translation: UseTranslationResponse<'data-table', any>,
opts?: { isOwner?: boolean; hydrated?: boolean },
): ColumnDef<SenseBox>[] {
const hydrated = opts?.hydrated ?? false
// ...
}```
2 participants
Type of Change
Implementation
Checklist
devbranchAdditional Information
date-fnsdependency with browser-native implementation #1061 date format mismatch on my devices page #1062