fix: VTIMEZONE-based timezone resolution and recurring event fixes#479
Conversation
…m timezone IDs Outlook sometimes emits meaningless TZID values like "Customized Time Zone" instead of a real IANA identifier. node-ical previously fell back to guessLocalZone(), which breaks on systems where the host timezone isn't the calendar's timezone (e.g. Homey hubs always run in UTC). The accompanying VTIMEZONE section contains the actual STANDARD/DAYLIGHT offsets. We now use those as a fingerprint to find a matching IANA zone (e.g. -05:00/-04:00 → America/Detroit), so recurring events spanning DST transitions are also handled correctly. Results are cached; the initial scan costs ~14 ms, subsequent lookups < 0.01 ms. Falls back to a fixed offset when no IANA zone matches, and to guessLocalZone() when no VTIMEZONE is present. Fixes jens-maus#478
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds VTIMEZONE→IANA resolution and caching, threads a dateOnly flag through RRuleCompatWrapper with ZonedDateTime→Date conversion, moves tz-utils into Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Parser as ical.js
participant VCal as VTIMEZONE (ICS)
participant TZUtils as lib/tz-utils.js
participant Temporal as Temporal API
participant RRule as RRuleCompatWrapper
App->>Parser: parse ICS with custom TZID
Parser->>VCal: extract VTIMEZONE block
Parser->>TZUtils: resolveVTimezoneToIana(vTimezone, refYear)
TZUtils->>TZUtils: pickApplicableBlock(...) and cache lookup
TZUtils->>Temporal: probe Jan/Jul offsets for candidate IANA zones
Temporal-->>TZUtils: return zone offsets
TZUtils-->>Parser: { iana, offset } or fallback
Parser->>RRule: new RRuleCompatWrapper(rruleTemporal, dateOnly)
RRule->>RRule: `#zdtToDate`(zonedDateTime)
alt dateOnly
RRule-->>Parser: Date at local midnight (tagged dateOnly)
else
RRule-->>Parser: Date from epoch milliseconds
end
Parser-->>App: events with resolved timezone/recurrence
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Around line 394-412: The VTIMEZONE → IANA resolution logic should be extracted
and reused inside fallbackWithStackTimezone() so both branches use the same
resolved zone; locate the stackVTimezone discovery and the call to
tzUtil.resolveVTimezoneToIana(year), store its resolved.iana or resolved.offset
(or fallback to tzUtil.guessLocalZone()) in a local variable (e.g.,
resolvedZone) and then use that resolvedZone instead of calling resolveTZID() or
guessLocalZone() again when assigning tz or handling DTSTART with TZID; update
references to originalTz, tz and any resolveTZID(...) call to prefer
resolvedZone when stackVTimezone exists.
In `@tz-utils.js`:
- Around line 499-546: resolveVTimezoneToIana currently picks the first
STANDARD/DAYLIGHT blocks and only compares Jan/Jul offsets, which can choose
wrong zones when a VTIMEZONE contains multiple era-specific observances or zones
share offsets but have different transition dates; update the function to (1)
select the STANDARD/DAYLIGHT sub-components that actually apply to the requested
year by evaluating each component's DTSTART/RRULE/RDATE/EXDATE and
tzoffsetfrom/tzoffsetto (use the existing vTimezone/components, standard,
daylight symbols to locate them), (2) compute the actual transition instants for
that year from those observance rules and derive the expected offset
before/after each transition, (3) when iterating getZoneNames() compare the
candidate IANA zone's real transition instants/offsets in that same year (not
just probeJan/probeJul) against the VTIMEZONE transitions and only accept a zone
if the transition instants and resulting offsets match, otherwise fall back to
the fixed-offset branch (minutesToEtcZone/stdOffset) and do not cache a
misleading IANA mapping; also ensure the cacheKey remains per
stdMins|dstMins|year but only store the result when the observance match is
exact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60d8a902-7cf3-475d-8c74-869b52fcbd5d
📒 Files selected for processing (4)
ical.jstest/advanced.test.jstest/tz-utils.test.jstz-utils.js
DATE-only (VALUE=DATE) recurring events were broken for hosts west of UTC because two independent assumptions about UTC midnight did not hold in those timezones: 1. RRuleCompatWrapper converted every rrule-temporal ZonedDateTime result via `new Date(zdt.epochMilliseconds)`. For VALUE=DATE events rrule-temporal anchors occurrences to UTC midnight, so e.g. 2025-01-01T00:00:00Z displays as 2024-12-31 in America/New_York — the wrong calendar date. Fix: add a `dateOnly` flag to RRuleCompatWrapper and a `#zdtToDate()` helper that, for date-only events, builds the Date from the ZDT's calendar components (`new Date(zdt.year, zdt.month-1, zdt.day)`) and sets `dateOnly=true` on the result. 2. `adjustSearchRange` passed local-midnight boundaries to `rrule.between()`. In UTC-5 "2025-01-01 00:00 local" is 2025-01-01T05:00:00Z, which is *after* the UTC-midnight anchor of the first occurrence — so the first day was silently excluded. Fix: for full-day events, normalise both bounds to UTC midnight with `Date.UTC(y, m, d)` so the comparison is host-TZ-independent. Update two test assertions that relied on UTC getters (`getUTCFullYear` etc.) for DATE-only dates, and fix the `handles negative duration` assertion to compare local calendar dates (`toDateString()`) rather than UTC ISO strings. Fixes 7 test failures under TZ=America/New_York.
…om MS TZIDs
fallbackWithStackTimezone() is the code path taken when DTSTART carries no
TZID parameter at all. It found the VTIMEZONE on the parser stack and then
called resolveTZID(tzid), but resolveTZID replaces custom Microsoft TZID
strings ("Customized Time Zone ...", "tzone://Microsoft/...") with
guessLocalZone() unconditionally — so the VTIMEZONE's STANDARD/DAYLIGHT
offset rules were silently ignored and the host's local timezone was used
instead.
The fix mirrors the explicit-TZID branch (issue jens-maus#478): check whether the
TZID is a custom Microsoft name before calling resolveTZID; if so, invoke
resolveVTimezoneToIana() on the VTIMEZONE to extract an IANA zone (or
fixed-offset fallback) from the STANDARD/DAYLIGHT rules first.
Add test fixture floating-dtstart-custom-vtimezone.ics and a test that
verifies a DTSTART with no TZID parameter is resolved to the correct UTC
instant when the calendar carries a matching custom-name VTIMEZONE.
…imezoneToIana A VTIMEZONE can contain multiple historic observance blocks for the same component type — e.g. Exchange emits both a pre-2007 and a post-2007 STANDARD/DAYLIGHT pair for US Eastern time. The previous code used .find() which returned whichever block happened to appear first in the (UUID-keyed) object. For an event in 2025 this could silently pick the 1967 observance rule and compute the wrong UTC offset. Fix: replace .find() with pickApplicableBlock(), a small helper that selects the block whose DTSTART year is the largest one \u2264 the event year (i.e. the most recent rule already in effect). Falls back to the oldest block when all blocks start after the reference year. Add fixture multi-era-vtimezone.ics and test that verifies both a modern (2025) and a vintage (1985) event are resolved to the correct UTC instant when the VTIMEZONE contains two STANDARD and two DAYLIGHT blocks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/tz-utils.js`:
- Around line 479-480: There is a duplicate JSDoc opener (`/**`) in
lib/tz-utils.js (two consecutive `/**` at the start of a comment) which produces
a malformed JSDoc; remove the extra `/**` so that only a single `/**` begins the
comment block (e.g., edit the JSDoc immediately above the related function in
tz-utils.js to have one `/**`), ensuring the rest of the comment and its closing
`*/` remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b230de0-f4cb-4508-b38c-20fe20c5b71f
📒 Files selected for processing (6)
ical.jslib/date-utils.jslib/tz-utils.jstest/advanced.test.jstest/basic.test.jstest/tz-utils.test.js
✅ Files skipped from review due to trivial changes (1)
- lib/date-utils.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/tz-utils.test.js
Consistent with lib/date-utils.js; tz-utils is internal-only (not exported through node-ical.js, not mentioned in README or .d.ts) so this is not a breaking change. Update all internal require paths accordingly.
b689faf to
62c7492
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/tz-utils.js`:
- Around line 550-587: The fallback currently always returns stdOffset even when
dstMins is finite (DST zone), which misparses summer times; change the fallback
behavior in the vtimezone lookup (the block that currently creates fallback =
{iana: undefined, offset: stdOffset}) so that when dstMins is finite and differs
from stdMins you do not unconditionally return stdOffset — instead either (A)
pick the active offset based on the event's local date/time by accepting/using a
localDate/localDateTime parameter (or the Instant to test) and return offset =
stdOffset or dstOffset accordingly, or (B) return no fixed offset (e.g. {iana:
undefined, offset: undefined} or include both offsets) so parseWithOffset can
decide from the event datetime; update vtimezoneIanaCache semantics accordingly
and document/reference parseWithOffset to ensure callers handle the
undefined/multi-offset response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e65f1782-482a-4ba1-bfc2-63ca2f45da43
📒 Files selected for processing (6)
ical.jslib/date-utils.jslib/tz-utils.jstest/advanced.test.jstest/basic.test.jstest/tz-utils.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/basic.test.js
- lib/date-utils.js
…ils for DST zone
When resolveVTimezoneToIana cannot find any IANA zone that matches the
VTIMEZONE's STD/DST offset pair, returning stdOffset is silently wrong for
roughly half of all timestamps (those that fall in the DST period).
Return {iana: undefined, offset: undefined} instead so callers fall back to
floating/local time rather than applying a confident wrong offset. Add a
console.warn to surface the case immediately should it ever occur in practice
(it shouldn't — every real DST zone is in the Temporal/Intl database).
The /* eslint-env */ directive is no longer recognised under ESLint flat config and will become an error in v10. xo already provides Mocha globals for *.test.js files automatically, so the comments were redundant.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Around line 164-171: The before(date, inclusive = false) and after(date,
inclusive = false) methods currently return null when no occurrence is found;
change them to return undefined instead to match the public type Date |
undefined — i.e., where they currently return result ? this.#zdtToDate(result) :
null, replace the null branch with undefined so both before and after (and any
similar early-return logic) return undefined when result is falsy while still
converting via `#zdtToDate`(result) when present.
- Around line 159-162: RRuleCompatWrapper.all currently forwards the iterator
directly to the underlying RRuleTemporal.all which passes Temporal.ZonedDateTime
to the iterator; wrap any provided iterator so it receives a Date instead by
converting its incoming ZonedDateTime argument with the existing `#zdtToDate`
helper before calling the user iterator, and then pass that wrapped iterator to
this._rrule.all; if no iterator is provided, call this._rrule.all() as before,
and continue to map the final results with this.#zdtToDate as the method already
does.
In `@lib/tz-utils.js`:
- Around line 516-517: Update the JSDoc for the exported helper that returns
{iana, offset} (the helper in lib/tz-utils.js which produces an object with iana
and offset properties) to accurately describe its contract: document that it
returns an object of shape { iana: string | undefined, offset: string |
undefined }; when an IANA zone is found iana is a string and offset may be
undefined, when a non‑DST VTIMEZONE can only be represented as a fixed UTC
offset the helper returns iana: undefined and offset: "<±HH:MM>", and when the
VTIMEZONE observes DST but cannot be matched the helper returns iana: undefined
and offset: undefined. Ensure the JSDoc replaces the old “falls back to a fixed
UTC-offset string” wording with these three explicit cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90399807-eef0-4078-9334-0de71e507626
📒 Files selected for processing (6)
ical.jslib/tz-utils.jstest/date-only-rrule-until.test.jstest/google-calendar-until-bug.test.jstest/monthly-bymonthday-multiple.test.jstest/non-utc-until.test.js
💤 Files with no reviewable changes (3)
- test/monthly-bymonthday-multiple.test.js
- test/non-utc-until.test.js
- test/google-calendar-until-bug.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/date-only-rrule-until.test.js
- before()/after(): return undefined instead of null when no occurrence is found, matching the declared return type (Date | undefined) - all(iterator): wrap the caller's iterator so it receives a converted Date rather than a raw Temporal.ZonedDateTime, consistent with the rest of the wrapper's public surface Also update the JSDoc for resolveVTimezoneToIana to accurately describe its three return cases after the DST-fallback change (ad84e01).
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Around line 374-389: Summary: The code currently forces fallback to
tzUtil.guessLocalZone() when resolveVTimezoneToIana() returns no iana/offset,
causing valid VTIMEZONE-backed custom TZIDs to collapse to the host zone. Fix:
when vTimezone exists, call tzUtil.resolveVTimezoneToIana(vTimezone, year) for
any unresolved/resolvable TZID (use variables resolvedTzId, vTimezone,
resolveVTimezoneToIana), and only use the resolver's iana or offset if present;
otherwise keep the original resolvedTzId (do not substitute
tzUtil.guessLocalZone() here). Then pass that result into
tzUtil.resolveTZID(...) (tzInfo) so that an unresolved result is preserved
rather than coercing to guessLocalZone().
In `@lib/tz-utils.js`:
- Around line 535-576: In resolveVTimezoneToIana, add an explicit guard for the
incoming year (e.g., coerce to Number and ensure Number.isFinite and
Number.isInteger / valid numeric year) immediately after the vTimezone
validation and before building cacheKey or calling Temporal.Instant.from; if the
year is invalid, return {iana: undefined, offset: undefined} so the function
fails closed and never uses an undefined/NaN year in cacheKey or in the
probeJan/probeJul Temporal.Instant.from calls.
ical.js – fallbackWithStackTimezone:
- Remove narrow isCustomMsTz guard; instead attempt
resolveVTimezoneToIana whenever a VTIMEZONE block is present.
Any TZID can benefit from embedded STANDARD/DAYLIGHT offset data,
not just the handful of known Microsoft custom strings.
- Drop the guessLocalZone() fallback: when VTIMEZONE resolution
returns neither iana nor offset, preserve the original resolvedTzId
so resolveTZID() can make a best effort or the floating-time path
at the end of the function applies.
lib/tz-utils.js – resolveVTimezoneToIana:
- Add explicit year guard immediately after the vTimezone check.
Coerce to Number and reject non-finite / non-integer values with
{ iana: undefined, offset: undefined } so that an invalid year
can never corrupt the cache key or cause Temporal.Instant.from()
to throw on a malformed ISO string.
- Use the coerced yearNumber throughout (pickApplicableBlock,
cacheKey, probeJan/probeJul, console.warn).
When a matching VTIMEZONE block exists but resolveVTimezoneToIana()
returns { iana: undefined, offset: undefined } (no IANA zone matched
the Jan/Jul offset probes), tz was silently replaced with the host
timezone — recreating the wrong-instant bug on UTC hosts.
Mirror the fix already applied in fallbackWithStackTimezone(): only
replace tz when resolution actually yields a value; otherwise leave
the original tz in place so resolveTZID() can make a best effort.
guessLocalZone() is only substituted when no VTIMEZONE is present at
all.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ical.js (1)
163-164: Minor: misleading parameter name in iterator wrapper.The second parameter passed by
RRuleTemporal.all()is the 0-based occurrence index (i), not a length. Consider renaming for clarity.📝 Suggested rename
const wrappedIterator = iterator - ? (zdt, length) => iterator(this.#zdtToDate(zdt), length) + ? (zdt, index) => iterator(this.#zdtToDate(zdt), index) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ical.js` around lines 163 - 164, The wrapper for iterator passed into RRuleTemporal.all() uses a misleading parameter name "length" even though the second argument is the 0-based occurrence index; update the wrapper (wrappedIterator and its use of iterator) to rename the second parameter to "i" or "index" and forward that value to iterator(this.#zdtToDate(zdt), i) so callers and future readers see the correct semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ical.js`:
- Around line 163-164: The wrapper for iterator passed into RRuleTemporal.all()
uses a misleading parameter name "length" even though the second argument is the
0-based occurrence index; update the wrapper (wrappedIterator and its use of
iterator) to rename the second parameter to "i" or "index" and forward that
value to iterator(this.#zdtToDate(zdt), i) so callers and future readers see the
correct semantics.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ical.js (1)
441-446: Consider simplifying the VTIMEZONE lookup.The nested logic for matching the VTIMEZONE by
tzidis correct but somewhat dense. The combination offlatMap,find, conditional array handling, and string normalization makes it harder to follow.♻️ Optional: Extract VTIMEZONE lookup into a helper function
+// Helper to find a VTIMEZONE block in the stack by its TZID +function findVTimezoneByTzid(stack, targetTzid) { + const normalizedTarget = String(targetTzid).replace(/^"(.*)"$/, '$1'); + return (stack || []) + .flatMap(item => Object.values(item)) + .find(v => { + if (!v || v.type !== 'VTIMEZONE') return false; + const tzids = Array.isArray(v.tzid) ? v.tzid : [String(v.tzid)]; + return tzids + .map(id => String(id).replace(/^"(.*)"$/, '$1')) + .includes(normalizedTarget); + }); +} // In the explicit-TZID branch: - const stackVTimezone = (stack || []) - .flatMap(item => Object.values(item)) - .find(v => v && v.type === 'VTIMEZONE' - && (Array.isArray(v.tzid) ? v.tzid : [String(v.tzid)]) - .map(id => String(id).replace(/^"(.*)"$/, '$1')) - .includes(originalTz)); + const stackVTimezone = findVTimezoneByTzid(stack, originalTz);This would also allow reuse in
fallbackWithStackTimezone()if needed in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ical.js` around lines 441 - 446, The VTIMEZONE lookup in the stack is dense; extract it into a small helper (e.g., findVtimezoneInStack(stack, originalTz)) that flattens stack entries, normalizes tzid values (ensure it's an array, coerce to strings, strip surrounding quotes with /^"(.*)"$/, and compare to originalTz) and returns the matching VTIMEZONE object; then replace the inline logic that sets stackVTimezone with a call to findVtimezoneInStack(stack, originalTz) and reuse this helper in fallbackWithStackTimezone() if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ical.js`:
- Around line 441-446: The VTIMEZONE lookup in the stack is dense; extract it
into a small helper (e.g., findVtimezoneInStack(stack, originalTz)) that
flattens stack entries, normalizes tzid values (ensure it's an array, coerce to
strings, strip surrounding quotes with /^"(.*)"$/, and compare to originalTz)
and returns the matching VTIMEZONE object; then replace the inline logic that
sets stackVTimezone with a call to findVtimezoneInStack(stack, originalTz) and
reuse this helper in fallbackWithStackTimezone() if needed.
The VTIMEZONE lookup was inlined twice in different forms: - fallbackWithStackTimezone: two-step find → separate object-values call - explicit-TZID branch: dense flatMap/find with inline TZID normalisation Replace both with a shared findVtimezoneInStack(stack, tzid) at module level. With tzid provided, it returns the VTIMEZONE whose (quote-stripped) tzid matches; without tzid it returns the first VTIMEZONE found (semantics needed by the floating-DTSTART branch). The explicit stack parameter makes the helper testable independently of the dateParameter closure.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@jens-maus Coderabbit seems satisfied 🙂 |
Originally a targeted fix for #478 (Outlook TZID
Customized Time Zone→ wrong host timezone), grown significantly through reviews and deeper code analysis.What changed
Core fix (
lib/tz-utils.js):resolveVTimezoneToIana(): usesSTANDARD/DAYLIGHToffsets as a fingerprint for IANA lookup (-05:00/-04:00→America/Detroit); results cachedpickApplicableBlock(): when multiple observance blocks exist (e.g. US pre-/post-2007 DST rules), reliably picks the most recent block withDTSTART≤ event year instead of the first one found{ iana: undefined, offset: undefined }instead ofstdOffset(which would be wrong for ~50 % of timestamps in the DST period)yearvalues are rejected early before they can corrupt the cache key or causeTemporal.Instant.from()to throwVTIMEZONE resolution in the parser (
ical.js):fallbackWithStackTimezone(TZID-less DTSTART): was using the same brokenguessLocalZone()logic — now uses VTIMEZONE resolutionguessLocalZone()on failed resolution — original TZID is now preserved soresolveTZIDcan make a best effortguessLocalZone()is only substituted when noVTIMEZONEblock is presentFurther fixes:
RRuleCompatWrapper:before()/after()returnednullinstead ofundefined;all(iterator)passed rawTemporal.ZonedDateTimeto the caller instead ofDateRefactor:
tz-utils.js→lib/tz-utils.jsFixes #478
Summary by CodeRabbit
Bug Fixes
New Features
Tests