[WEB-4277]fix: timezone conversion for cycles dates#7180
Closed
vamsikrishnamathala wants to merge 3 commits intopreviewfrom
Closed
[WEB-4277]fix: timezone conversion for cycles dates#7180vamsikrishnamathala wants to merge 3 commits intopreviewfrom
vamsikrishnamathala wants to merge 3 commits intopreviewfrom
Conversation
Contributor
WalkthroughThe cycle list item component was simplified by removing unused hook variables. Two new date formatting functions were added to the utilities module, providing standardized payload formatting and a safe user-friendly formatter with fallback support. Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/helpers/date-time.helper.ts (1)
479-512: LGTM! Robust implementation with comprehensive fallback handling.The function effectively addresses the timezone conversion issues mentioned in the PR objectives by leveraging the existing
renderFormattedPayloadDateandgetDateutilities that handle timezone-safe parsing. The defensive programming approach with multiple validation layers and try-catch error handling ensures reliable date formatting.Consider this minor optimization to reduce the double conversion overhead:
export const renderSafeFormattedDate = ( date: Date | string | undefined | null, formatToken: string = "MMM dd, yyyy", fallback: string = "Invalid date" ): string => { if (!date) return fallback; - // Use renderFormattedPayloadDate to get a properly formatted payload date - const payloadDate = renderFormattedPayloadDate(date); - - // If renderFormattedPayloadDate returns undefined/null, return fallback - if (!payloadDate) return fallback; - try { - // Parse and format the payload date - const parsedDate = getDate(payloadDate); + // Parse the date directly using the timezone-safe getDate function + const parsedDate = getDate(date); if (!parsedDate || !isValid(parsedDate)) return fallback; return format(parsedDate, formatToken); } catch (error) { // Return fallback if any error occurs during formatting return fallback; } };However, the current implementation ensures maximum consistency with the existing payload date normalization pipeline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/core/components/cycles/list/cycle-list-item-action.tsx(2 hunks)web/helpers/date-time.helper.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/core/components/cycles/list/cycle-list-item-action.tsx (1)
web/helpers/date-time.helper.ts (1)
renderSafeFormattedDate(489-512)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
web/core/components/cycles/list/cycle-list-item-action.tsx (2)
26-26: LGTM! Correct import of the new safe date formatter.The import correctly includes the new
renderSafeFormattedDatehelper function.
233-238: Excellent improvement! Safe date formatting with proper fallback handling.The replacement of direct
format(parseISO(...), "MMM dd, yyyy")calls withrenderSafeFormattedDateprovides several benefits:
- Error resilience: Handles invalid or undefined dates gracefully instead of throwing errors
- Timezone safety: Leverages the timezone-safe parsing pipeline via
getDatefunction- Consistent fallback: Shows "Invalid date" for problematic dates instead of crashing
- Maintained format: Preserves the existing "MMM dd, yyyy" display format
This directly addresses the PR objective of fixing timezone conversion issues for cycle dates.
anmolsinghbhatia
approved these changes
Jun 6, 2025
sriramveeraghanta
requested changes
Jun 8, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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/cycles/list/cycle-list-item-action.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/core/components/cycles/list/cycle-list-item-action.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/utils/src/datetime.ts (1)
web/helpers/date-time.helper.ts (2)
renderFormattedPayloadDate(58-68)getDate(277-293)
Comment on lines
+355
to
+388
| /** | ||
| * @returns {string} safely formatted date or fallback text | ||
| * @description Safely formats a date using renderFormattedPayloadDate and date-fns format, with fallback for invalid dates | ||
| * @param {Date | string | undefined | null} date | ||
| * @param {string} formatToken (optional) // default "MMM dd, yyyy" | ||
| * @param {string} fallback (optional) // default "Invalid date" | ||
| * @example renderSafeFormattedDate("2024-01-01") // "Jan 01, 2024" | ||
| * @example renderSafeFormattedDate(null) // "Invalid date" | ||
| * @example renderSafeFormattedDate("2024-01-01", "MM/dd/yyyy", "N/A") // "01/01/2024" | ||
| */ | ||
| export const renderSafeFormattedDate = ( | ||
| date: Date | string | undefined | null, | ||
| formatToken: string = "MMM dd, yyyy", | ||
| fallback: string = "Invalid date" | ||
| ): string => { | ||
| if (!date) return fallback; | ||
|
|
||
| // Use renderFormattedPayloadDate to get a properly formatted payload date | ||
| const payloadDate = renderFormattedPayloadDate(date); | ||
|
|
||
| // If renderFormattedPayloadDate returns undefined/null, return fallback | ||
| if (!payloadDate) return fallback; | ||
|
|
||
| try { | ||
| // Parse and format the payload date | ||
| const parsedDate = getDate(payloadDate); | ||
| if (!parsedDate || !isValid(parsedDate)) return fallback; | ||
|
|
||
| return format(parsedDate, formatToken); | ||
| } catch (error) { | ||
| // Return fallback if any error occurs during formatting | ||
| return fallback; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate double parsing and improve efficiency.
The current implementation parses and validates the date twice - once in renderFormattedPayloadDate and again in this function. This is inefficient and unnecessarily complex.
Consider this simplified implementation that eliminates double parsing:
-export const renderSafeFormattedDate = (
- date: Date | string | undefined | null,
- formatToken: string = "MMM dd, yyyy",
- fallback: string = "Invalid date"
-): string => {
- if (!date) return fallback;
-
- // Use renderFormattedPayloadDate to get a properly formatted payload date
- const payloadDate = renderFormattedPayloadDate(date);
-
- // If renderFormattedPayloadDate returns undefined/null, return fallback
- if (!payloadDate) return fallback;
-
- try {
- // Parse and format the payload date
- const parsedDate = getDate(payloadDate);
- if (!parsedDate || !isValid(parsedDate)) return fallback;
-
- return format(parsedDate, formatToken);
- } catch (error) {
- // Return fallback if any error occurs during formatting
- return fallback;
- }
-};
+export const renderSafeFormattedDate = (
+ date: Date | string | undefined | null,
+ formatToken: string = "MMM dd, yyyy",
+ fallback: string = "Invalid date"
+): string => {
+ if (!date) return fallback;
+
+ try {
+ // Parse the date once
+ const parsedDate = getDate(date);
+ if (!parsedDate || !isValid(parsedDate)) return fallback;
+
+ return format(parsedDate, formatToken);
+ } catch (error) {
+ // Return fallback if any error occurs during formatting
+ return fallback;
+ }
+};This approach:
- Eliminates the unnecessary intermediate payload format step
- Reduces parsing from twice to once
- Simplifies the logic while maintaining the same safety guarantees
- Is more performant and easier to understand
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * @returns {string} safely formatted date or fallback text | |
| * @description Safely formats a date using renderFormattedPayloadDate and date-fns format, with fallback for invalid dates | |
| * @param {Date | string | undefined | null} date | |
| * @param {string} formatToken (optional) // default "MMM dd, yyyy" | |
| * @param {string} fallback (optional) // default "Invalid date" | |
| * @example renderSafeFormattedDate("2024-01-01") // "Jan 01, 2024" | |
| * @example renderSafeFormattedDate(null) // "Invalid date" | |
| * @example renderSafeFormattedDate("2024-01-01", "MM/dd/yyyy", "N/A") // "01/01/2024" | |
| */ | |
| export const renderSafeFormattedDate = ( | |
| date: Date | string | undefined | null, | |
| formatToken: string = "MMM dd, yyyy", | |
| fallback: string = "Invalid date" | |
| ): string => { | |
| if (!date) return fallback; | |
| // Use renderFormattedPayloadDate to get a properly formatted payload date | |
| const payloadDate = renderFormattedPayloadDate(date); | |
| // If renderFormattedPayloadDate returns undefined/null, return fallback | |
| if (!payloadDate) return fallback; | |
| try { | |
| // Parse and format the payload date | |
| const parsedDate = getDate(payloadDate); | |
| if (!parsedDate || !isValid(parsedDate)) return fallback; | |
| return format(parsedDate, formatToken); | |
| } catch (error) { | |
| // Return fallback if any error occurs during formatting | |
| return fallback; | |
| } | |
| }; | |
| export const renderSafeFormattedDate = ( | |
| date: Date | string | undefined | null, | |
| formatToken: string = "MMM dd, yyyy", | |
| fallback: string = "Invalid date" | |
| ): string => { | |
| if (!date) return fallback; | |
| try { | |
| // Parse the date once | |
| const parsedDate = getDate(date); | |
| if (!parsedDate || !isValid(parsedDate)) return fallback; | |
| return format(parsedDate, formatToken); | |
| } catch (error) { | |
| // Return fallback if any error occurs during formatting | |
| return fallback; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In packages/utils/src/datetime.ts lines 355 to 388, the function
renderSafeFormattedDate currently parses and validates the date twice, once
inside renderFormattedPayloadDate and again in this function, causing
inefficiency. Refactor the function to parse and validate the date only once by
removing the call to renderFormattedPayloadDate and directly parsing the input
date, then validating and formatting it. This will simplify the logic, improve
performance, and maintain the same safety guarantees.
Member
Author
|
This issue is fixed by Merged Dates. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This update renders the safe formatted date, avoiding unnecessary date conversions for the start date and end date in cycles.
Type of Change
Summary by CodeRabbit
New Features
Refactor