issue#1034: Enhance date formatting in Home component to support date ranges#1047
issue#1034: Enhance date formatting in Home component to support date ranges#1047arkid15r merged 12 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
WalkthroughThis pull request updates the date formatting logic for upcoming events in the Home component and its associated tests. A new utility function, Changes
Suggested reviewers
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/utils/dateFormatter.ts (2)
18-40: Consider handling additional edge cases in formatDateRange.The implementation of
formatDateRangehandles the basic scenarios well but doesn't address some edge cases:
- What if
endDateis beforestartDate?- What if the dates are the same? (Currently would format as "Month Day - Day, Year" which is redundant)
Consider adding these additional checks:
export const formatDateRange = (startDate: number | string, endDate: number | string) => { const start = typeof startDate === 'number' ? new Date(startDate * 1000) : new Date(startDate) const end = typeof endDate === 'number' ? new Date(endDate * 1000) : new Date(endDate) if (isNaN(start.getTime()) || isNaN(end.getTime())) { throw new Error('Invalid date') } + // If dates are identical or end is before start, just format as a single date + if (start.getTime() === end.getTime() || end < start) { + return formatDate(startDate) + } // Check if dates are in the same month and year const sameMonth = start.getMonth() === end.getMonth() && start.getFullYear() === end.getFullYear() const sameYear = start.getFullYear() === end.getFullYear() if (sameMonth) { // Format as "Month Day - Day, Year" (e.g., "Sep 1 - 4, 2025") return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` } else if (sameYear) { // Different months but same year (e.g., "Sep 29 - Oct 2, 2025") return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.toLocaleDateString('en-US', { month: 'short' })} ${end.getDate()}, ${start.getFullYear()}` } else { // Different years (e.g., "Dec 30, 2025 - Jan 3, 2026") return `${start.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })} - ${end.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })}` } }
30-39: Consider extracting formatting options to reduce duplication.The formatting options are repeated in multiple places. Consider extracting them into variables to make the code more maintainable.
export const formatDateRange = (startDate: number | string, endDate: number | string) => { const start = typeof startDate === 'number' ? new Date(startDate * 1000) : new Date(startDate) const end = typeof endDate === 'number' ? new Date(endDate * 1000) : new Date(endDate) if (isNaN(start.getTime()) || isNaN(end.getTime())) { throw new Error('Invalid date') } // Check if dates are in the same month and year const sameMonth = start.getMonth() === end.getMonth() && start.getFullYear() === end.getFullYear() const sameYear = start.getFullYear() === end.getFullYear() + const shortMonthFormat = { month: 'short' }; + const fullDateFormat = { month: 'short', day: 'numeric', year: 'numeric' }; if (sameMonth) { // Format as "Month Day - Day, Year" (e.g., "Sep 1 - 4, 2025") - return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` + return `${start.toLocaleDateString('en-US', shortMonthFormat)} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` } else if (sameYear) { // Different months but same year (e.g., "Sep 29 - Oct 2, 2025") - return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.toLocaleDateString('en-US', { month: 'short' })} ${end.getDate()}, ${start.getFullYear()}` + return `${start.toLocaleDateString('en-US', shortMonthFormat)} ${start.getDate()} - ${end.toLocaleDateString('en-US', shortMonthFormat)} ${end.getDate()}, ${start.getFullYear()}` } else { // Different years (e.g., "Dec 30, 2025 - Jan 3, 2026") - return `${start.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })} - ${end.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })}` + return `${start.toLocaleDateString('en-US', fullDateFormat)} - ${end.toLocaleDateString('en-US', fullDateFormat)}` } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/pages/Home.test.tsx(2 hunks)frontend/src/pages/Home.tsx(2 hunks)frontend/src/utils/dateFormatter.ts(1 hunks)
🔇 Additional comments (5)
frontend/src/pages/Home.tsx (2)
21-21: Import of formatDateRange is correctly added.The import statement has been updated to include the new
formatDateRangefunction alongside the existingformatDatefunction.
150-152: Improved date range formatting logic.The conditional logic has been enhanced to use the new
formatDateRangefunction when an event spans multiple days, providing a more concise and user-friendly display for date ranges.frontend/__tests__/unit/pages/Home.test.tsx (2)
7-7: Import of formatDateRange is correctly added to test file.The import statement has been updated to include the new
formatDateRangefunction alongside the existingformatDatefunction.
164-167: Test logic properly updated to match implementation.The test now correctly verifies the date formatting behavior based on whether the event has different start and end dates, ensuring the component's new date formatting functionality is properly tested.
frontend/src/utils/dateFormatter.ts (1)
22-24: Error handling is consistent with formatDate.The validation for invalid dates follows the same pattern as in the
formatDatefunction, which is good for consistency.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Thanks for addressing this, however I'm concerned about you working on issues assigned to other people. This is the last one that is going to be accepted as not assigned to you.
Please look into these when you get a chance:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/utils/dateFormatter.ts (1)
39-52: Suggestion: Refactor to reduce code duplication.The date formatting logic could be extracted into helper functions to reduce duplication and improve maintainability.
+ // Helper function to format date parts + const formatDateParts = (date: Date, options: Intl.DateTimeFormatOptions) => { + return date.toLocaleDateString('en-US', options); + } // Check if dates are in the same month and year const sameMonth = start.getMonth() === end.getMonth() && start.getFullYear() === end.getFullYear() const sameYear = start.getFullYear() === end.getFullYear() if (sameMonth) { // Format as "Month Day - Day, Year" (e.g., "Sep 1 - 4, 2025") - return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` + return `${formatDateParts(start, { month: 'short' })} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` } else if (sameYear) { // Different months but same year (e.g., "Sep 29 - Oct 2, 2025") - return `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.toLocaleDateString('en-US', { month: 'short' })} ${end.getDate()}, ${start.getFullYear()}` + return `${formatDateParts(start, { month: 'short' })} ${start.getDate()} - ${formatDateParts(end, { month: 'short' })} ${end.getDate()}, ${start.getFullYear()}` } else { // Different years (e.g., "Dec 30, 2025 - Jan 3, 2026") - return `${start.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })} - ${end.toLocaleDateString('en-US', { month: 'short', day: 'numeric', year: 'numeric' })}` + return `${formatDateParts(start, { month: 'short', day: 'numeric', year: 'numeric' })} - ${formatDateParts(end, { month: 'short', day: 'numeric', year: 'numeric' })}` }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/__tests__/unit/utils/dateFormatter.test.ts(1 hunks)frontend/src/utils/dateFormatter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/pages/Home.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (3)
frontend/__tests__/e2e/pages/Home.spec.ts (1)
74-74: Updated test expectation matches the new date range formatting.The test has been appropriately updated to check for the consolidated date range format "Feb 27 - 28, 2025" instead of separate date strings, which aligns with the new date formatting approach implemented in the Home component.
frontend/__tests__/unit/utils/dateFormatter.test.ts (1)
1-85: Comprehensive test coverage for date formatting functions.The test suite provides thorough coverage for both
formatDateandformatDateRangefunctions, testing various scenarios including:
- Different date formats (ISO strings, Unix timestamps)
- Error handling for invalid dates
- Date ranges in the same month, different months, and different years
- Edge cases like month/year boundaries and single-day ranges
This addresses the previous reviewer's request for unit tests and ensures the new functionality is properly validated.
frontend/src/utils/dateFormatter.ts (1)
18-53: Well-implemented date range formatting function.The
formatDateRangefunction effectively handles different date range scenarios with appropriate conditional logic:
- Same day → returns a single date format
- Same month → "Month Day - Day, Year" format
- Same year, different months → "Month Day - Month Day, Year" format
- Different years → "Month Day, Year - Month Day, Year" format
The function also properly handles both ISO date strings and Unix timestamps with appropriate error checking.
|
I apologize for this mistake. I didn’t know about the assignment rule when I made the pull request but realized it after asking in last night’s huddle. I’ll be more careful next time. Thanks for letting me know! |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Looks good, please check these:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/utils/dateFormatter.ts (3)
22-27: Consider refactoring duplicated date parsing logic.This date parsing logic duplicates code from the
formatDatefunction. Consider extracting this into a private helper function to follow the DRY principle.+const parseDate = (input: number | string): Date => { + return typeof input === 'number' + ? new Date(input * 1000) // Unix timestamp in seconds + : new Date(input) // ISO date string +} export const formatDate = (input: number | string) => { - const date = - typeof input === 'number' - ? new Date(input * 1000) // Unix timestamp in seconds - : new Date(input) // ISO date string + const date = parseDate(input) // Rest of the function... } export const formatDateRange = ( startDate: number | string, endDate: number | string ) => { - const start = typeof startDate === 'number' - ? new Date(startDate * 1000) - : new Date(startDate) - const end = typeof endDate === 'number' - ? new Date(endDate * 1000) - : new Date(endDate) + const start = parseDate(startDate) + const end = parseDate(endDate) // Rest of the function... }
29-31: Error message could be more specific.The current error message doesn't indicate which date is invalid (start date, end date, or both).
if (isNaN(start.getTime()) || isNaN(end.getTime())) { - throw new Error('Invalid date') + if (isNaN(start.getTime()) && isNaN(end.getTime())) { + throw new Error('Both start and end dates are invalid') + } else if (isNaN(start.getTime())) { + throw new Error('Invalid start date') + } else { + throw new Error('Invalid end date') + } }
47-53: Combine string concatenation into a single template literal.Using multiple string concatenations can be less readable. Consider using a single template literal for improved readability.
if (sameMonth) { // Format as "Month Day - Day, Year" (e.g., "Sep 1 - 4, 2025") return ( - `${start.toLocaleDateString('en-US', { month: 'short' })} ` + - `${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` + `${start.toLocaleDateString('en-US', { month: 'short' })} ${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/dateFormatter.ts(1 hunks)
🔇 Additional comments (5)
frontend/src/utils/dateFormatter.ts (5)
18-21: Function signature is well-defined and consistent with formatDate.The new
formatDateRangefunction maintains the same parameter type flexibility as the existingformatDatefunction, accepting both Unix timestamps (number) and date strings.
33-40: Good implementation for handling same-day events.The approach of reusing
formatDatefor same-day events is efficient and correctly implements the specified behavior. This effectively addresses the previous review comment "It seems you can reuseformatDatehere".
61-61: LGTM! Clean implementation of same-year formatting.The code correctly formats dates from different months in the same year, using readable variable names and a concise template literal.
64-64: Excellent reuse of the existing formatDate function.For different years, reusing the existing
formatDatefunction is the right approach - it maintains consistency and follows the DRY principle.
18-66: Verify that unit tests cover all edge cases.As mentioned in a previous review comment, please ensure that your unit tests in
frontend/__tests__/unit/utils/dateFormatter.test.tscover all edge cases including:
- Invalid dates
- Same day events
- Same month events
- Same year but different month events
- Different year events
- Leap year dates
- Edge of month/year transitions
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/utils/dateFormatter.ts (1)
41-43: 🛠️ Refactor suggestionSupport internationalization by adding a locale parameter
The function hardcodes 'en-US' locale for date formatting. For better internationalization support, consider accepting a locale parameter that defaults to 'en-US' but can be overridden.
-export const formatDateRange = (startDate: number | string, endDate: number | string) => { +export const formatDateRange = ( + startDate: number | string, + endDate: number | string, + locale: string = 'en-US' +) => { // ...existing code... if (sameMonth) { // Format as "Month Day - Day, Year" (e.g., "Sep 1 - 4, 2025") return ( - `${start.toLocaleDateString('en-US', { month: 'short' })} ` + + `${start.toLocaleDateString(locale, { month: 'short' })} ` + `${start.getDate()} - ${end.getDate()}, ${start.getFullYear()}` ) } else if (sameYear) { // Different months but same year (e.g., "Sep 29 - Oct 2, 2025") - const startMonth = start.toLocaleDateString('en-US', { month: 'short' }) - const endMonth = end.toLocaleDateString('en-US', { month: 'short' }) + const startMonth = start.toLocaleDateString(locale, { month: 'short' }) + const endMonth = end.toLocaleDateString(locale, { month: 'short' }) // ... } else { // Different years (e.g., "Dec 30, 2025 - Jan 3, 2026") - return `${formatDate(startDate)} - ${formatDate(endDate)}` + return `${formatDate(startDate, locale)} - ${formatDate(endDate, locale)}` }Also update the
formatDatefunction to accept a locale parameter:-export const formatDate = (input: number | string) => { +export const formatDate = (input: number | string, locale: string = 'en-US') => { // ...existing code... - return date.toLocaleDateString('en-US', { + return date.toLocaleDateString(locale, { year: 'numeric', month: 'short', day: 'numeric', })
🧹 Nitpick comments (2)
frontend/src/utils/dateFormatter.ts (2)
18-20: Consider extracting the date parsing logic to a helper functionThere's duplicated date parsing logic between
formatDateandformatDateRange. Extracting this logic into a helper function would make the code more maintainable and reduce duplication.+// Helper function to parse different date input formats +const parseDate = (input: number | string): Date => { + return typeof input === 'number' + ? new Date(input * 1000) // Unix timestamp in seconds + : new Date(input) // ISO date string +} export const formatDate = (input: number | string) => { - const date = - typeof input === 'number' - ? new Date(input * 1000) // Unix timestamp in seconds - : new Date(input) // ISO date string + const date = parseDate(input) if (isNaN(date.getTime())) { throw new Error('Invalid date') } // ... rest of function unchanged export const formatDateRange = (startDate: number | string, endDate: number | string) => { - const start = typeof startDate === 'number' ? new Date(startDate * 1000) : new Date(startDate) - const end = typeof endDate === 'number' ? new Date(endDate * 1000) : new Date(endDate) + const start = parseDate(startDate) + const end = parseDate(endDate)
22-24: Add validation for invalid date range orderThe function validates that both dates are valid but doesn't check if the end date is after the start date. Consider adding validation to handle cases where the end date precedes the start date.
if (isNaN(start.getTime()) || isNaN(end.getTime())) { throw new Error('Invalid date') } + +// Ensure end date is not before start date +if (end.getTime() < start.getTime()) { + throw new Error('End date cannot be before start date') +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/utils/dateFormatter.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (3)
frontend/src/utils/dateFormatter.ts (3)
35-37: Good use of variables for clarityThe code defines clear variable names (
sameMonthandsameYear) which improves readability and makes the conditional logic easier to understand.
32-33: Good code reuse with formatDateExcellent decision to reuse the existing
formatDatefunction when the dates are the same. This follows the DRY principle and ensures consistent formatting.
55-56: Good reuse of formatDate for different years caseWell implemented handling of different years by leveraging the existing
formatDatefunction. This ensures consistency in the formatting of individual dates within the range.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/utils/dateFormatter.ts (1)
41-42: Consider consistent formatting for date ranges.You're using em dashes (—) as separators between dates. While typographically correct, ensure this is consistent with the rest of the application's date range formatting. If other parts of the UI use hyphens (-) instead, you might want to standardize.
Also applies to: 52-52, 55-55
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/pages/Home.test.tsx(1 hunks)frontend/__tests__/unit/utils/dateFormatter.test.ts(1 hunks)frontend/src/pages/Home.tsx(2 hunks)frontend/src/utils/dateFormatter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/unit/pages/Home.test.tsx
- frontend/tests/e2e/pages/Home.spec.ts
- frontend/src/pages/Home.tsx
- frontend/tests/unit/utils/dateFormatter.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (2)
frontend/src/utils/dateFormatter.ts (2)
18-57: Well-structured implementation with good edge case handling.The
formatDateRangefunction effectively handles various date range scenarios, reusing the existingformatDatefunction where appropriate. The code is clean, easy to follow, and includes helpful comments.
41-42: Enhance internationalization support for date formatting.The function currently hardcodes the 'en-US' locale. For better internationalization support, consider accepting a locale parameter that defaults to 'en-US' but can be overridden.
What are best practices for internationalization in JavaScript date formatting?Also applies to: 46-48, 55-55
… ranges (OWASP#1047) * issue#1034: Enhance date formatting in Home component to support date ranges * Enhance date formatting functions and add unit tests for date utilities * Update Upcoming Events date formatting in tests to reflect range display * Refactor date range formatting for improved readability and maintainability * Refactor formatDateRange function for improved readability * Update code --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Kate Golovanova <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
3 participants
Related Issue
Closes #1034
Summary
This PR improves event date formatting by introducing a new function,
formatDateRange, which:Sep 1, 2025 - Sep 4, 2025→Sep 1 - 4, 2025).Home.tsxcomponent and its corresponding tests inHome.test.ts.Screenshots
🛠️ Changes Made
Code Enhancements
formatDateRangefunction:Home.tsx:formatDateorformatDateRangebased on event duration.Home.test.ts:Test Result