fix: timezone parsing issues for calendars #21665
Conversation
|
@curiousgeorgios is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (06/01/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (06/01/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
cubic found 2 issues across 2 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
packages/lib/CalendarService.ts
Outdated
| // Fix timezone extraction - get TZID from DTSTART property parameters | ||
| const dtstartProperty = vevent.getFirstProperty("dtstart"); | ||
| const tzidFromDtstart = dtstartProperty?.getParameter("tzid"); | ||
|
|
There was a problem hiding this comment.
Line contains only whitespace which violates project formatting guidelines and adds unnecessary diff noise.
…n methods accepting the cubic-dev-ai as implement backward compatibility with different event ICS event types Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
thanks @Devanshusharma2005 - updated. |
|
@curiousgeorgios getting typecheck issues. |
|
This PR is being marked as stale due to inactivity. |
|
marking this pr as closed due to inactivity. Feel free to ping me up if you want to continue to work on this. |
thank you brother |
|
@curiousgeorgios could you pls also attach the after image |
There was a problem hiding this comment.
cubic reviewed 2 files and found no issues. Review PR in cubic.dev.
yep will do that 👌 |
|
@curiousgeorgios A type check seems to be failing. Can you fix that? |
- Fix VTIMEZONE selection to find matching timezone instead of using first one - Improve TZID parameter extraction from DTSTART property using jCal array - Fix precedence: DTSTART TZID > standalone TZID > UTC > fallback - Resolves issue where Europe/Brussels events showed wrong times in Europe/Paris
Head branch was pushed to by a user without write access
WalkthroughThe changes focus on improving timezone handling and event time conversion logic within calendar service classes. The extraction of the timezone identifier (TZID) now gives precedence to the TZID parameter from the DTSTART property, accessed via its internal structure, rather than relying on a standalone TZID property or defaulting to UTC. When selecting the relevant VTIMEZONE component from the calendar data, the updated logic matches the component's TZID with the event's TZID, falling back to the first available VTIMEZONE if no match is found. Additionally, event start and end times are converted to ISO strings once and stored in variables before being added to the results, reducing redundant conversions. No public or exported entity signatures are changed. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/app-store/ics-feedcalendar/lib/CalendarService.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. packages/lib/CalendarService.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-playwright". (The package "eslint-plugin-playwright" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-playwright" was referenced from the config file in ".eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. ✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/app-store/ics-feedcalendar/lib/CalendarService.ts (1)
162-165: Apply the same type safety improvements as in BaseCalendarService.This code has the same jCal array access pattern that could benefit from error handling.
- // Fix timezone extraction - get TZID from DTSTART property parameters - const dtstartProperty = vevent.getFirstProperty("dtstart"); - // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value] - const tzidFromDtstart = dtstartProperty ? (dtstartProperty as any).jCal[1].tzid : undefined; + // Fix timezone extraction - get TZID from DTSTART property parameters + const dtstartProperty = vevent.getFirstProperty("dtstart"); + // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value] + let tzidFromDtstart: string | undefined; + try { + tzidFromDtstart = dtstartProperty && (dtstartProperty as any).jCal?.[1]?.tzid; + } catch (e) { + console.error("Error extracting TZID from DTSTART property:", e); + tzidFromDtstart = undefined; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/app-store/ics-feedcalendar/lib/CalendarService.ts(3 hunks)packages/lib/CalendarService.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/app-store/ics-feedcalendar/lib/CalendarService.ts (1)
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.738Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
packages/lib/CalendarService.ts (1)
Learnt from: hariombalhara
PR: calcom/cal.com#22547
File: packages/embeds/embed-core/src/lib/eventHandlers/scrollByDistanceEventHandler.ts:11-14
Timestamp: 2025-07-16T11:46:28.738Z
Learning: In Cal.com's embed system, internal events like "__scrollByDistance" are fired by Cal.com's own code, so runtime validation of event data structure is unnecessary since TypeScript type system guarantees type safety for internal events.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: apply-labels-from-issue
- GitHub Check: Security Check
- GitHub Check: Security Check
🔇 Additional comments (4)
packages/lib/CalendarService.ts (3)
405-407: LGTM! Correct timezone precedence implementation.The updated TZID precedence logic correctly prioritizes the DTSTART parameter while maintaining proper fallbacks, addressing the timezone parsing issues mentioned in the PR objectives.
436-447: Excellent improvement to VTIMEZONE selection logic.The code now correctly searches for a VTIMEZONE component that matches the event's TZID instead of always using the first one. This ensures proper timezone handling when multiple timezones are defined in the calendar.
519-524: Good refactoring for readability and performance.Caching the ISO string conversions in variables improves code readability and avoids redundant conversions.
packages/app-store/ics-feedcalendar/lib/CalendarService.ts (1)
203-289: Excellent consistency with BaseCalendarService implementation.The timezone handling improvements are consistently implemented across both calendar service classes, ensuring uniform behavior for timezone parsing throughout the application.
| // Fix timezone extraction - get TZID from DTSTART property parameters | ||
| const dtstartProperty = vevent.getFirstProperty("dtstart"); | ||
| // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value] | ||
| const tzidFromDtstart = dtstartProperty ? (dtstartProperty as any).jCal[1].tzid : undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety and error handling for jCal array access.
The code correctly extracts TZID from DTSTART parameters, but accessing the internal jCal structure without proper type checking could lead to runtime errors if the array structure is unexpected.
Consider adding error handling:
- // Fix timezone extraction - get TZID from DTSTART property parameters
- const dtstartProperty = vevent.getFirstProperty("dtstart");
- // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value]
- const tzidFromDtstart = dtstartProperty ? (dtstartProperty as any).jCal[1].tzid : undefined;
+ // Fix timezone extraction - get TZID from DTSTART property parameters
+ const dtstartProperty = vevent.getFirstProperty("dtstart");
+ // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value]
+ let tzidFromDtstart: string | undefined;
+ try {
+ tzidFromDtstart = dtstartProperty && (dtstartProperty as any).jCal?.[1]?.tzid;
+ } catch (e) {
+ console.error("Error extracting TZID from DTSTART property:", e);
+ tzidFromDtstart = undefined;
+ }📝 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.
| // Fix timezone extraction - get TZID from DTSTART property parameters | |
| const dtstartProperty = vevent.getFirstProperty("dtstart"); | |
| // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value] | |
| const tzidFromDtstart = dtstartProperty ? (dtstartProperty as any).jCal[1].tzid : undefined; | |
| // Fix timezone extraction - get TZID from DTSTART property parameters | |
| const dtstartProperty = vevent.getFirstProperty("dtstart"); | |
| // In ical.js, parameters are accessible through the property's jCal array structure: [name, params, type, value] | |
| let tzidFromDtstart: string | undefined; | |
| try { | |
| tzidFromDtstart = dtstartProperty && (dtstartProperty as any).jCal?.[1]?.tzid; | |
| } catch (e) { | |
| console.error("Error extracting TZID from DTSTART property:", e); | |
| tzidFromDtstart = undefined; | |
| } |
🤖 Prompt for AI Agents
In packages/lib/CalendarService.ts around lines 396 to 399, the code accesses
the internal jCal array of the dtstartProperty without type safety or checks,
which risks runtime errors if the structure is not as expected. To fix this, add
type checks to ensure dtstartProperty.jCal exists, is an array, and has the
expected elements before accessing the tzid property. Also, include error
handling or fallback logic if the tzid is not found or the structure is invalid
to prevent crashes.
|
hey guys turns should be all working and good to go now: have tested as per the commit and here is the after pics and timezones lining up perfectly. Summary of the events: Timeline: ICS Feed Timezone Handling BugIssue Discovery
Root Cause InvestigationBug #1: Incorrect VTIMEZONE Selection // BROKEN: Always used first VTIMEZONE regardless of event's actual timezone
const vtimezone = vcalendar.getFirstSubcomponent("vtimezone");
Bug #2: Failed TZID Parameter Extraction // BROKEN: Attempted to extract timezone from non-existent property
const timezone = dtstart ? dtstart["timezone"] : undefined;
Solution ImplementationFix #1: Smart VTIMEZONE Matching // FIXED: Find VTIMEZONE that matches event's TZID
let vtimezone = null;
if (tzid) {
const allVtimezones = vcalendar.getAllSubcomponents("vtimezone");
vtimezone = allVtimezones.find((vtz) => vtz.getFirstPropertyValue("tzid") === tzid);
}
if (!vtimezone) {
vtimezone = vcalendar.getFirstSubcomponent("vtimezone"); // Fallback
}Fix #2: Correct TZID Parameter Extraction // FIXED: Use ical.js jCal array structure for parameter access
const dtstartProperty = vevent.getFirstProperty("dtstart");
const tzidFromDtstart = dtstartProperty ? (dtstartProperty as any).jCal[1].tzid : undefined;Fix #3: Proper Fallback Priority // FIXED: Complete fallback chain with correct precedence
const tzid = tzidFromDtstart || vevent?.getFirstPropertyValue("tzid") || (isUTC ? "UTC" : timezone);Files Modified
Testing
Result
This fix resolves timezone handling issues for ICS calendar feeds and ensures events display at the correct local times. Screenshots after the fix: 
|
|
@anikdhabal grateful for your review to try and get this merged |
What does this PR do?
Visual Demo (For contributors especially)
Below is a troubleshooting event that I setup on my ICS calendar provider
And this event is showing as 0300 to 0400 (in the same timezone settings in cal)
Video Demo (if applicable):
Not applicable
Image Demo (if applicable):
See screenshots above
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
Summary by cubic
Fixed calendar timezone parsing to correctly handle TZID values from ICS files, preventing event time discrepancies.