fix: #321 use date-fns for localized datetime formatting #345
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Assessment against linked issues
Possibly related PRs
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
frontend/composables/use-formatters.ts (2)
41-75: OptimizegetLocale()calls by caching the resultThe
getLocale()function is called multiple times withinfmtDate, which may introduce unnecessary overhead. Consider retrieving the locale once and reusing it within the function scope.You can modify the
fmtDatefunction as follows:export function fmtDate( value: string | Date, fmt: DateTimeFormat = "human", type: DateTimeType = "date" ): string { + const locale = getLocale(); const dt = typeof value === "string" ? new Date(value) : value; if (!dt || !validDate(dt)) { return ""; } if (fmt === "relative") { - return useLocaleTimeAgo(dt); + return formatDistance(dt, new Date(), { locale }); } if (type === "time") { - return format(dt, "p", { locale: getLocale() }); + return format(dt, "p", { locale }); } let formatStr = ""; switch (fmt) { case "human": formatStr = "PPP"; break; case "long": formatStr = "PP"; break; case "short": formatStr = "P"; break; default: return ""; } if (type === "datetime") { formatStr += "p"; } - return format(dt, formatStr, { locale: getLocale() }); + return format(dt, formatStr, { locale }); }
38-38: Ensure consistent time zones when formatting relative timeIn
useLocaleTimeAgo, you're calculating the distance between the input date and the current date usingnew Date(). Ensure that both dates are in the same time zone to prevent inaccuracies in the relative time display.
🛑 Comments failed to post (4)
frontend/composables/use-formatters.ts (4)
44-44:
⚠️ Potential issueUndefined function
validDateThe function
validDate(dt)is called but not defined within this file. Ensure thatvalidDateis properly defined or imported to prevent runtime errors.
41-44:
⚠️ Potential issueValidate date inputs to prevent potential security risks
When parsing date strings using
new Date(value), ensure that the input is validated and sanitized. Malformed or malicious inputs could lead to unexpected behavior or security vulnerabilities.
30-34:
⚠️ Potential issueValidate locale codes to prevent potential runtime errors
The
getLocalefunction uses locale codes derived from user input, which might be invalid or unsupported. Consider adding validation to ensure thatlocaleCodecorresponds to an available locale to prevent runtime errors.You could add a validation step:
function getLocale() { const t = useI18n(); const localeCode = t?.locale?.value as string ?? "en-US"; + const supportedLocales = ['en-US', 'hu']; + if (!supportedLocales.includes(localeCode)) { + localeCode = 'en-US'; + } const region = localeCode.length > 2 ? localeCode.substring(3) : ""; return (Locales[(localeCode.substring(0,2) + region) as keyof typeof Locales] ?? Locales[localeCode.substring(0,2) as keyof typeof Locales] ?? Locales.enUS); }Committable suggestion skipped: line range outside the PR's diff.
2-3: 🛠️ Refactor suggestion
Consider importing only necessary locales to reduce bundle size
Importing all locales from
date-fns/localecan significantly increase your bundle size, impacting application performance. It's recommended to import only the locales you need.You can modify the import statements as follows:
-import * as Locales from "date-fns/locale"; +import enUS from "date-fns/locale/en-US"; +import hu from "date-fns/locale/hu";And update the
getLocalefunction accordingly:function getLocale() { const t = useI18n(); const localeCode = t?.locale?.value as string ?? "en-US"; const region = localeCode.length > 2 ? localeCode.substring(3) : ""; - return (Locales[(localeCode.substring(0,2) + region) as keyof typeof Locales] ?? Locales[localeCode.substring(0,2) as keyof typeof Locales] ?? Locales.enUS); + const localesMap = { 'enUS': enUS, 'hu': hu }; + const localeKey = (localeCode.substring(0, 2) + region) || 'enUS'; + return localesMap[localeKey] || enUS; }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
frontend/composables/use-formatters.ts (2)
31-38: Consider caching the locale result.The locale resolution logic is correct, but consider caching the result to avoid repeated calculations on every format operation.
+const localeCache = new Map<string, Locale>(); function getLocale() { const t = useI18n(); const localeCode = (t?.locale?.value as string) ?? "en-US"; + if (localeCache.has(localeCode)) { + return localeCache.get(localeCode); + } const lang = localeCode.length > 1 ? localeCode.substring(0, 2) : localeCode; const region = localeCode.length > 2 ? localeCode.substring(3) : ""; - return ( + const locale = ( Locales[(lang + region) as keyof typeof Locales] ?? Locales[lang as keyof typeof Locales] ?? Locales.enUS ); + localeCache.set(localeCode, locale); + return locale; }
45-79: Extract format strings as constants.The implementation looks good, but consider extracting the format strings as named constants for better maintainability and reusability.
+const FORMAT_PATTERNS = { + HUMAN: 'PPP', + LONG: 'PP', + SHORT: 'P', + TIME: 'p' +} as const; export function fmtDate(value: string | Date, fmt: DateTimeFormat = "human", type: DateTimeType = "date"): string { // ... existing validation ... if (type === "time") { - return format(dt, "p", { locale: getLocale() }); + return format(dt, FORMAT_PATTERNS.TIME, { locale: getLocale() }); } let formatStr = ""; switch (fmt) { case "human": - formatStr = "PPP"; + formatStr = FORMAT_PATTERNS.HUMAN; break; case "long": - formatStr = "PP"; + formatStr = FORMAT_PATTERNS.LONG; break; case "short": - formatStr = "P"; + formatStr = FORMAT_PATTERNS.SHORT; break; // ... rest of the function
🛑 Comments failed to post (1)
frontend/composables/use-formatters.ts (1)
42-42: 🛠️ Refactor suggestion
Add date validation check.
The function should validate the input date to prevent potential runtime errors.
export function useLocaleTimeAgo(date: Date) { + if (!date || !validDate(date)) { + return ""; + } return formatDistance(date, new Date(), { locale: getLocale() }); }Committable suggestion skipped: line range outside the PR's diff.
What type of PR is this?
What this PR does / why we need it:
Displays dates and times in correctly localized formats via date-fns functions.
Which issue(s) this PR fixes:
Fixes #321
Summary by CodeRabbit
New Features
Bug Fixes
Refactor