[WEB-3374]feat: added merge date display #7141
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UIComponent
participant DateRangeDropdown
participant MergedDateDisplay
User->>UIComponent: Interacts with date field
UIComponent->>DateRangeDropdown: Render with mergeDates/isClearable
DateRangeDropdown->>MergedDateDisplay: Display merged date range
User->>DateRangeDropdown: Selects or clears date range
DateRangeDropdown->>UIComponent: onSelect callback with new/empty range
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![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)
web/core/components/dropdowns/merged-date.tsx (1)
28-72: Consider adding validation for date order and edge cases.The formatting logic is solid, but there are a few edge cases to consider:
- Date order validation: The component doesn't validate if
startDateis beforeendDate- Same date scenario: When both dates are identical, it could show just one date instead of "Jan 24 - 24, 2025"
const formatDateRange = (): string => { // If no dates are provided if (!parsedStartDate && !parsedEndDate) { return ""; } // If only start date is provided if (parsedStartDate && !parsedEndDate) { return format(parsedStartDate, "MMM dd, yyyy"); } // If only end date is provided if (!parsedStartDate && parsedEndDate) { return format(parsedEndDate, "MMM dd, yyyy"); } // If both dates are provided if (parsedStartDate && parsedEndDate) { + // Handle same date scenario + if (parsedStartDate.getTime() === parsedEndDate.getTime()) { + return format(parsedStartDate, "MMM dd, yyyy"); + } + const startYear = parsedStartDate.getFullYear(); const startMonth = parsedStartDate.getMonth(); const endYear = parsedEndDate.getFullYear(); const endMonth = parsedEndDate.getMonth(); // Rest of the logic... } };web/core/components/dropdowns/date-range.tsx (1)
195-204: Consider accessibility improvements for the clear button.The clear functionality works well, but could benefit from accessibility enhancements:
{isClearable && !disabled && hasDisplayedDates && ( <X className={cn("h-2.5 w-2.5 flex-shrink-0 cursor-pointer", clearIconClassName)} + role="button" + aria-label="Clear date range" + tabIndex={0} onClick={(e) => { e.stopPropagation(); e.preventDefault(); clearDates(); }} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.stopPropagation(); + e.preventDefault(); + clearDates(); + } + }} /> )}Also applies to: 228-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
web/core/components/cycles/analytics-sidebar/sidebar-header.tsx(2 hunks)web/core/components/cycles/list/cycle-list-item-action.tsx(3 hunks)web/core/components/dropdowns/date-range.tsx(6 hunks)web/core/components/dropdowns/index.ts(1 hunks)web/core/components/dropdowns/merged-date.tsx(1 hunks)web/core/components/issues/issue-detail-widgets/sub-issues/issues-list/properties.tsx(3 hunks)web/core/components/issues/issue-layouts/properties/all-properties.tsx(3 hunks)web/core/components/modules/module-list-item-action.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
web/core/components/cycles/list/cycle-list-item-action.tsx (1)
web/core/components/dropdowns/merged-date.tsx (1)
MergedDateDisplay(20-81)
web/core/components/issues/issue-detail-widgets/sub-issues/issues-list/properties.tsx (3)
web/core/store/router.store.ts (2)
workspaceSlug(69-71)issueId(149-151)web/helpers/date-time.helper.ts (2)
renderFormattedPayloadDate(58-68)getDate(277-293)web/core/components/dropdowns/date-range.tsx (1)
DateRangeDropdown(63-285)
web/core/components/dropdowns/date-range.tsx (1)
web/core/components/dropdowns/merged-date.tsx (1)
MergedDateDisplay(20-81)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (20)
web/core/components/modules/module-list-item-action.tsx (1)
161-161: LGTM! Consistent implementation of merged dates feature.The addition of the
mergeDatesprop toDateRangeDropdownis correctly implemented and consistent with the pattern used across other components in this PR.web/core/components/dropdowns/index.ts (1)
6-6: LGTM! Necessary export for the new component.The export statement correctly makes the
MergedDateDisplaycomponent available for use across the application.web/core/components/cycles/list/cycle-list-item-action.tsx (3)
25-25: LGTM! Proper import for the new component.The import statement correctly brings in the
MergedDateDisplaycomponent needed for the refactoring.
233-233: Excellent refactoring! Replaced inline formatting with reusable component.The replacement of inline date formatting with the
MergedDateDisplaycomponent improves code maintainability and ensures consistent date formatting across the application. The component receives the correctstartDateandendDateprops.
270-270: LGTM! Consistent implementation of merged dates feature.The addition of the
mergeDatesprop toDateRangeDropdownmaintains consistency with the pattern implemented across other components in this PR.web/core/components/cycles/analytics-sidebar/sidebar-header.tsx (3)
6-6: Good cleanup! Removed unused imports.Cleaning up unused imports improves code maintainability and reduces bundle size.
11-11: Good cleanup! Removed unused imports.Removing unused imports helps keep the codebase clean and maintainable.
233-233: LGTM! Consistent implementation of merged dates feature.The addition of the
mergeDatesprop toDateRangeDropdownis correctly implemented and consistent with the pattern used across other components in this PR.web/core/components/dropdowns/merged-date.tsx (1)
20-81: Excellent implementation with comprehensive date formatting logic!The
MergedDateDisplaycomponent provides a clean and well-structured approach to displaying date ranges with smart formatting based on the relationship between start and end dates. The logic correctly handles all major scenarios and provides intuitive date displays.web/core/components/issues/issue-detail-widgets/sub-issues/issues-list/properties.tsx (4)
7-7: Clean import updates for the new component.The import statement correctly adds
DateRangeDropdownto replace the previous date handling approach.
39-53: Well-structured date handlers with proper validation.The separate handlers for start and target dates are cleanly implemented with appropriate null checking and date formatting. The use of
renderFormattedPayloadDateensures consistent date format for API calls.
107-134: Excellent integration of merged date functionality.The implementation successfully consolidates start and due date handling into a single component with several good practices:
- Proper use of
WithDisplayPropertiesHOCwith multiple display property keys- Correct date range construction from issue properties
- Appropriate button variant selection based on date presence
- Good event propagation handling
The
mergeDatesprop andisClearablefeature enhance the user experience significantly.
114-121:✅ Verification successful
Verify the onSelect handler behavior with the DateRangeDropdown.
The
onSelecthandler assumes that the range object may have undefined values forfromandto, but it's worth verifying that the DateRangeDropdown component handles partial selections correctly.
🏁 Script executed:
#!/bin/bash # Description: Verify DateRangeDropdown onSelect behavior and type safety # Search for DateRangeDropdown implementation to understand onSelect callback signature ast-grep --pattern $'export const DateRangeDropdown: React.FC<Props> = (props) => { $$$ onSelect: (range: DateRange | undefined) => void; $$$ }' # Check for other usages of DateRangeDropdown onSelect to ensure consistency rg -A 5 -B 2 "onSelect.*range"Length of output: 2535
🏁 Script executed:
#!/bin/bash # Inspect DateRange type definition rg -nE "export (?:type|interface) DateRange" web/core/components/dropdowns/date-range.tsx rg -nE -A3 -B3 "export (?:type|interface) DateRange" web/core/components/dropdowns/date-range.tsx # Find where and how onSelect is invoked inside DateRangeDropdown rg -n "props.onSelect" -n web/core/components/dropdowns/date-range.tsx rg -n "onSelect\(" -n web/core/components/dropdowns/date-range.tsxLength of output: 811
🏁 Script executed:
#!/bin/bash # Locate DateRange type/interface in the dropdown component rg -n "type DateRange" -n web/core/components/dropdowns/date-range.tsx rg -n "interface DateRange" -n web/core/components/dropdowns/date-range.tsx # Show the beginning of the file to inspect Props and onSelect invocation sed -n '1,200p' web/core/components/dropdowns/date-range.tsxLength of output: 6044
🏁 Script executed:
#!/bin/bash # Locate Calendar usage to see onSelect handling inside DateRangeDropdown rg -n "<Calendar" -n web/core/components/dropdowns/date-range.tsx # Print lines around Calendar and apply/cancel buttons sed -n '200,400p' web/core/components/dropdowns/date-range.tsxLength of output: 3181
No issues found with DateRangeDropdown’s onSelect handling
The
DateRangeDropdownsimply forwards theDateRange | undefinedvalue it receives from the underlying Calendar to itsonSelectprop, and your handler correctly mapsrange?.fromandrange?.to(falling back tonull) for both full and partial selections. No further changes are needed here.web/core/components/issues/issue-layouts/properties/all-properties.tsx (3)
8-8: Clean import refactoring removes unused dependencies.The import updates correctly remove unused icons (
CalendarClock) and add the newDateRangeDropdowncomponent while maintaining all necessary dependencies.Also applies to: 24-24
307-337: Excellent consolidation of date handling logic.The refactoring successfully merges separate start and due date dropdowns into a unified
DateRangeDropdownwith several improvements:
- Maintains existing
handleStartDateandhandleTargetDatefunctions for proper event tracking- Correctly constructs date range from issue properties
- Preserves button styling logic based on date presence
- Retains existing conditional styling for overdue dates
- Adds clearable functionality for better UX
The implementation maintains backward compatibility while providing enhanced functionality.
315-322:✅ Verification successful
Verify date range selection behavior maintains existing functionality.
The unified date range selection approach looks correct, but ensure that the event tracking for individual date changes still works as expected since the events are captured separately in
handleStartDateandhandleTargetDate.
🏁 Script executed:
#!/bin/bash # Description: Verify that date tracking events are still properly captured # Search for ISSUE_UPDATED event usage in date handlers rg -A 10 -B 5 "ISSUE_UPDATED.*start_date|ISSUE_UPDATED.*target_date" # Check for any other date-related event tracking patterns rg -A 5 "changed_property.*date"Length of output: 2603
Date range selection: individual change events intact
Verified that bothstart_dateandtarget_dateanalytics events are still emitted:• web/core/components/issues/issue-layouts/properties/all-properties.tsx –
changed_property: "start_date"
• web/core/components/issues/issue-layouts/properties/all-properties.tsx –changed_property: "target_date"No further changes needed.
web/core/components/dropdowns/date-range.tsx (4)
34-41: Well-designed new props enhance component flexibility.The addition of
clearIconClassName,isClearable, andmergeDatesprops provides excellent configurability while maintaining backward compatibility through sensible defaults.Also applies to: 72-79
132-139: Clean implementation of clear functionality.The
clearDatesfunction andhasDisplayedDatesvariable provide a straightforward way to reset the date range with proper state management and callback invocation.
176-239: Excellent implementation of conditional rendering logic.The merged vs separate date display logic is well-structured with:
- Clean conditional rendering based on
mergeDatesprop- Proper integration of
MergedDateDisplaycomponent- Consistent clear icon placement for both modes
- Good event propagation handling for clear actions
- Proper placeholder handling
The implementation maintains the original functionality while adding the new merged display option seamlessly.
166-169: Improved tooltip content formatting.The tooltip content now correctly handles empty states by omitting placeholder text and only showing the dash separator when both dates are present, which provides a cleaner user experience.
| {cycleDetails.start_date && <span>{format(parseISO(cycleDetails.start_date), "MMM dd, yyyy")}</span>} | ||
| <ArrowRight className="h-3 w-3 flex-shrink-0 my-auto" /> | ||
| {cycleDetails.end_date && <span>{format(parseISO(cycleDetails.end_date), "MMM dd, yyyy")}</span>} | ||
| <MergedDateDisplay startDate={cycleDetails.start_date} endDate={cycleDetails.end_date} className="" /> |
There was a problem hiding this comment.
Could you remove this className if it's not being used.
prateekshourya29
added
ready to merge
and removed
changes required
requires approval
labels
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/utils/src/datetime.ts (1)
337-343: Enhance JSDoc documentation with validation behavior.The documentation is excellent but should mention the validation behavior and error handling.
/** * Formats merged date range display with smart formatting + * Returns empty string for invalid dates or when no valid dates are provided * - Single date: "Jan 24, 2025" * - Same year, same month: "Jan 24 - 28, 2025" * - Same year, different month: "Jan 24 - Feb 6, 2025" * - Different year: "Dec 28, 2024 - Jan 4, 2025" + * @param parsedStartDate - Start date (can be null/undefined) + * @param parsedEndDate - End date (can be null/undefined) + * @returns Formatted date range string or empty string if invalid */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/utils/src/datetime.ts(1 hunks)web/core/components/dropdowns/merged-date.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/dropdowns/merged-date.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
| export const formatDateRange = ( | ||
| parsedStartDate: Date | null | undefined, | ||
| parsedEndDate: Date | null | undefined | ||
| ): string => { | ||
| // If no dates are provided | ||
| if (!parsedStartDate && !parsedEndDate) { | ||
| return ""; | ||
| } | ||
|
|
||
| // If only start date is provided | ||
| if (parsedStartDate && !parsedEndDate) { | ||
| return format(parsedStartDate, "MMM dd, yyyy"); | ||
| } | ||
|
|
||
| // If only end date is provided | ||
| if (!parsedStartDate && parsedEndDate) { | ||
| return format(parsedEndDate, "MMM dd, yyyy"); | ||
| } | ||
|
|
||
| // If both dates are provided | ||
| if (parsedStartDate && parsedEndDate) { | ||
| const startYear = parsedStartDate.getFullYear(); | ||
| const startMonth = parsedStartDate.getMonth(); | ||
| const endYear = parsedEndDate.getFullYear(); | ||
| const endMonth = parsedEndDate.getMonth(); | ||
|
|
||
| // Same year, same month | ||
| if (startYear === endYear && startMonth === endMonth) { | ||
| const startDay = format(parsedStartDate, "dd"); | ||
| const endDay = format(parsedEndDate, "dd"); | ||
| return `${format(parsedStartDate, "MMM")} ${startDay} - ${endDay}, ${startYear}`; | ||
| } | ||
|
|
||
| // Same year, different month | ||
| if (startYear === endYear) { | ||
| const startFormatted = format(parsedStartDate, "MMM dd"); | ||
| const endFormatted = format(parsedEndDate, "MMM dd"); | ||
| return `${startFormatted} - ${endFormatted}, ${startYear}`; | ||
| } | ||
|
|
||
| // Different year | ||
| const startFormatted = format(parsedStartDate, "MMM dd, yyyy"); | ||
| const endFormatted = format(parsedEndDate, "MMM dd, yyyy"); | ||
| return `${startFormatted} - ${endFormatted}`; | ||
| } | ||
|
|
||
| return ""; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for robust date formatting.
The function logic is well-structured, but it lacks critical input validation that could lead to runtime errors.
Consider these improvements:
export const formatDateRange = (
parsedStartDate: Date | null | undefined,
parsedEndDate: Date | null | undefined
): string => {
+ // Validate input dates
+ const isStartValid = parsedStartDate && isValid(parsedStartDate);
+ const isEndValid = parsedEndDate && isValid(parsedEndDate);
+
// If no dates are provided
- if (!parsedStartDate && !parsedEndDate) {
+ if (!isStartValid && !isEndValid) {
return "";
}
// If only start date is provided
- if (parsedStartDate && !parsedEndDate) {
+ if (isStartValid && !isEndValid) {
+ try {
return format(parsedStartDate, "MMM dd, yyyy");
+ } catch (e) {
+ return "";
+ }
}
// If only end date is provided
- if (!parsedStartDate && parsedEndDate) {
+ if (!isStartValid && isEndValid) {
+ try {
return format(parsedEndDate, "MMM dd, yyyy");
+ } catch (e) {
+ return "";
+ }
}
// If both dates are provided
- if (parsedStartDate && parsedEndDate) {
+ if (isStartValid && isEndValid) {
+ try {
const startYear = parsedStartDate.getFullYear();
const startMonth = parsedStartDate.getMonth();
const endYear = parsedEndDate.getFullYear();
const endMonth = parsedEndDate.getMonth();
// Same year, same month
if (startYear === endYear && startMonth === endMonth) {
const startDay = format(parsedStartDate, "dd");
const endDay = format(parsedEndDate, "dd");
return `${format(parsedStartDate, "MMM")} ${startDay} - ${endDay}, ${startYear}`;
}
// Same year, different month
if (startYear === endYear) {
const startFormatted = format(parsedStartDate, "MMM dd");
const endFormatted = format(parsedEndDate, "MMM dd");
return `${startFormatted} - ${endFormatted}, ${startYear}`;
}
// Different year
const startFormatted = format(parsedStartDate, "MMM dd, yyyy");
const endFormatted = format(parsedEndDate, "MMM dd, yyyy");
return `${startFormatted} - ${endFormatted}`;
+ } catch (e) {
+ return "";
+ }
}
return "";
};Additionally, consider adding date order validation if the business logic requires start date to be before end date.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/utils/src/datetime.ts around lines 344 to 391, the formatDateRange
function lacks input validation and error handling which may cause runtime
errors. Add checks to ensure parsedStartDate and parsedEndDate are valid Date
objects before formatting. Also, implement validation to confirm that if both
dates are provided, the start date is not after the end date, and handle invalid
inputs gracefully by returning an empty string or an appropriate message.
Description
This PR merge start date and due date into a single component in work items, cycles and modules list.
Type of Change
Screenshots and Media (if applicable)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes