fix: monthly recurring events on multiple days per month could skip the start month#477
Merged
jens-maus merged 4 commits intojens-maus:masterfrom Mar 6, 2026
Merged
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors recurring-event expansion in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/monthly-bymonthday-multiple.test.js (1)
32-38: Tests exerciserrule.all()directly rather thanexpandRecurringEvent.These tests validate the rrule-temporal fix but don't exercise the refactored
expandRecurringEventfunction or its new helpers (isExcludedByExdate,buildRecurringInstance, etc.). Consider adding a test that usesexpandRecurringEventwith BYMONTHDAY rules to ensure the integration works end-to-end.// Example additional test using expandRecurringEvent: it('should work through expandRecurringEvent', function () { const parsed = ical.parseICS(icsData); const event = Object.values(parsed).find(e => e.type === 'VEVENT'); const instances = ical.expandRecurringEvent(event, { from: new Date('2026-02-01T00:00:00Z'), to: new Date('2026-05-31T23:59:59Z'), }); assert.strictEqual(instances.length, 9); assert.strictEqual(instances[0].start.getUTCDate(), 24); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/monthly-bymonthday-multiple.test.js` around lines 32 - 38, Add a test that exercises the refactored recurrence expansion end-to-end by calling expandRecurringEvent (instead of using rrule.all() directly) on the parsed VEVENT from ical.parseICS, passing a from/to range and asserting the expected instance count and specific start dates; this will also validate the new helpers (isExcludedByExdate, buildRecurringInstance) are integrated—replace or add to the existing assertions around event.rrule and recurrences with a call to ical.expandRecurringEvent(event, { from: <Date>, to: <Date> }) and assert instances.length and instances[0].start.getUTCDate() (and any other expected values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/monthly-bymonthday-multiple.test.js`:
- Around line 32-38: Add a test that exercises the refactored recurrence
expansion end-to-end by calling expandRecurringEvent (instead of using
rrule.all() directly) on the parsed VEVENT from ical.parseICS, passing a from/to
range and asserting the expected instance count and specific start dates; this
will also validate the new helpers (isExcludedByExdate, buildRecurringInstance)
are integrated—replace or add to the existing assertions around event.rrule and
recurrences with a call to ical.expandRecurringEvent(event, { from: <Date>, to:
<Date> }) and assert instances.length and instances[0].start.getUTCDate() (and
any other expected values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4018f52a-8a89-40ab-85c9-a1f904c5f4a6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
node-ical.jspackage.jsontest/monthly-bymonthday-multiple.test.js
KristjanESPERANTO
force-pushed
the
multiple
branch
from
0960789 to
f7765f8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
node-ical.js (1)
413-431: Minor inefficiency: dual-key storage causes redundant iterations for full-day events.Due to the dual-key EXDATE storage strategy (both
dateKeyand ISO string for DATE-TIME entries),Object.values(event.exdate)may return the sameDateobject reference multiple times. The comparison works correctly but performs redundant iterations.Consider using a
Setto dedupe or breaking after the first match is found (already done), which makes this acceptable as-is. Alternatively, you could iterateObject.keys()and skip ISO-string keys to avoid duplicate comparisons.♻️ Optional: Skip ISO-string keys to avoid redundant comparisons
if (isFullDay) { - for (const exdateValue of Object.values(event.exdate)) { - if (exdateValue instanceof Date && getDateKey(exdateValue) === dateKey) { + for (const [key, exdateValue] of Object.entries(event.exdate)) { + // Skip ISO-string keys (contain 'T') to avoid checking same Date twice + if (key.includes('T')) { + continue; + } + if (exdateValue instanceof Date && getDateKey(exdateValue) === dateKey) { return true; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node-ical.js` around lines 413 - 431, The isExcludedByExdate function can waste CPU comparing the same Date value twice because event.exdate stores both calendar-key and ISO-string keys; update the full-day branch in isExcludedByExdate to iterate Object.keys(event.exdate) and for each key skip keys that look like ISO strings (e.g., contain 'T' or match /^\d{4}-\d{2}-\d{2}T/), then retrieve the Date via event.exdate[key] and compare with getDateKey(exdateValue) === dateKey, returning true on first match; alternatively, dedupe values by pushing them into a Set before comparing—reference isExcludedByExdate, event.exdate, getDateKey, dateKey, and date.toISOString() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@node-ical.js`:
- Around line 413-431: The isExcludedByExdate function can waste CPU comparing
the same Date value twice because event.exdate stores both calendar-key and
ISO-string keys; update the full-day branch in isExcludedByExdate to iterate
Object.keys(event.exdate) and for each key skip keys that look like ISO strings
(e.g., contain 'T' or match /^\d{4}-\d{2}-\d{2}T/), then retrieve the Date via
event.exdate[key] and compare with getDateKey(exdateValue) === dateKey,
returning true on first match; alternatively, dedupe values by pushing them into
a Set before comparing—reference isExcludedByExdate, event.exdate, getDateKey,
dateKey, and date.toISOString() when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17d9a60d-eb72-4e22-9a07-39ce33035c32
📒 Files selected for processing (3)
node-ical.jspackage.jsontest/monthly-bymonthday-multiple.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/monthly-bymonthday-multiple.test.js
- package.json
Covers the bug fixed in rrule-temporal v1.4.7: when DTSTART fell on one of multiple BYMONTHDAY values and an earlier BYMONTHDAY value existed in the same month, the entire start month was skipped. Tests: - start month occurrences after DTSTART are included (Feb 24 + Feb 28) - no occurrences before DTSTART are generated (Feb 10 is excluded)
KristjanESPERANTO
force-pushed
the
multiple
branch
2 times, most recently
from
aab4708 to
70a5d79
Compare
Contributor
Author
|
@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 `@node-ical.js`:
- Around line 494-495: The override lookup currently only checks
event.recurrences?.[dateKey], which can mis-select for timed recurrences; update
the logic in the isOverride/instanceEvent resolution to try an ISO datetime key
first (e.g. isoKey derived from the instance's full ISO timestamp) and use
event.recurrences?.[isoKey] when present, then fall back to
event.recurrences?.[dateKey] if not found, preserving the includeOverrides
guard; set isOverride based on whether either lookup returns a value and set
instanceEvent to the matched override (isoKey match preferred) or event
otherwise.
70a5d79 to
19f1740
Compare
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 `@node-ical.js`:
- Line 433: The current check in the return statement uses event.exdate[dateKey]
which for timed recurrences can incorrectly exclude all same-day instances;
change the logic so that for timed events you only match the exact timestamp
(use event.exdate[date.toISOString()]) and for floating/all-day events use
event.exdate[dateKey]. Update the return in the function that currently does
"return Boolean(event.exdate[dateKey] || event.exdate[date.toISOString()]);" to
first detect whether the event is timed (has a time component/non-floating
start) and then only consult event.exdate[date.toISOString()] for timed events
and event.exdate[dateKey] for floating/all-day events.
19f1740 to
20659ed
Compare
Replace the monolithic processRecurringInstance with four single-purpose helpers: - isExcludedByExdate: EXDATE lookup logic - validateDateRange: input validation for from/to - adjustSearchRange: full-day end-of-day nudge + expandOngoing lookback - buildRecurringInstance: construct one instance for an RRULE-generated date isFullDay and baseDurationMs are now computed once in expandRecurringEvent and passed down, eliminating the duplicate derivation in the old implementation. Also raise max-params to 5 in xo config — adjustSearchRange and buildRecurringInstance legitimately need 5 positional parameters; forcing an options object would obscure required, ordered arguments without real benefit. Fixes two pre-existing bugs discovered during the refactor: - EXDATE precision (timed events): the old code matched EXDATE entries by date-only key, accidentally excluding every instance on that calendar day. isExcludedByExdate now matches by exact ISO timestamp; it falls back to the date-only key only when the EXDATE entry itself is DATE-only (dateOnly=true), which per RFC 5545 intentionally covers the whole day. - RECURRENCE-ID precision (timed events): the old code matched recurrence overrides by date-only key only. This commit improves that to ISO-key-first with a date-only fallback; a remaining edge case (two timed instances sharing a calendar date) is addressed in the following commit.
…instances Pre-existing bug: storeRecurrenceOverride (ical.js) stores every DATE-TIME RECURRENCE-ID under both an ISO key and a date-only key. The original code resolved overrides using only the date-only key; the previous commit improved this to ISO-key-first with a date-only fallback. The fallback is still wrong when two instances share a calendar date (e.g. BYHOUR=9,15): a miss on the ISO key unambiguously means "no override for this instance", because every properly stored DATE-TIME RECURRENCE-ID carries both keys. Falling back to dateKey would incorrectly apply one occurrence's override to a different occurrence on the same day. Fix: for timed events, use the ISO key only. Full-day events have no ISO key and continue to use dateKey. Also add regression tests for both bugs fixed across commits 3–4: - EXDATE: only the matching timed instance must be excluded when two share a date - RECURRENCE-ID: only the matching timed instance must receive the override
KristjanESPERANTO
force-pushed
the
multiple
branch
from
2fac7ab to
8f8302f
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
Coderabbit seems to be satisfied now! Oh boy, that was another interesting ride 😄 But it's a good example of how refactoring and further testing are worthwhile, as they reveal edge-case bugs (again also with the help of coderabbit) that might otherwise remain unfixed forever. The failing CI test seems to be an GitHub issue and not something about this PR changes. |
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
2 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.
Started as a one-line dependency bump for rrule-temporal v1.4.7. Writing regression tests led to a refactor of
expandRecurringEvent(complexity lint violation), which exposed two pre-existing bugs. A CodeRabbit review caught a third.All three bugs are latent: they only trigger when a timed recurring event produces more than one instance on the same calendar date (e.g. via
BYHOUR). Uncommon enough to go unnoticed, but the consequences — silently excluding or overriding the wrong instance — are real correctness issues.Commits
1.
chore: update rrule-temporal ^1.4.6 → ^1.4.7Fixes a bug where
FREQ=MONTHLYwith multipleBYMONTHDAYvalues skipped the entire start month when an earlierBYMONTHDAYvalue existed in the same month.DTSTART=2025-01-15, RRULE:FREQ=MONTHLY;BYMONTHDAY=1,15— old: first occurrence February 1 · new: January 15 ✓
Bumped explicitly (not just relying on
^) becausenpm installwith an existing lock file uses the locked version. Upstream: ggaabe/rrule-temporal#1102.
test: add regression for FREQ=MONTHLY with multiple BYMONTHDAY valuesThree tests in
test/monthly-bymonthday-multiple.test.js:BYMONTHDAYexistsDTSTARTical.expandRecurringEvent()3.
refactor: decompose expandRecurringEvent into focused helper functionsprocessRecurringInstancehad cyclomatic complexity 22 (lint max: 20). ExtractedisExcludedByExdate,validateDateRange,adjustSearchRange,buildRecurringInstance.Two pre-existing bugs fixed in the process:
4.
fix: RECURRENCE-ID override must not bleed into other same-day timed instancesPre-existing bug (same root cause as commit 3's EXDATE fix):
storeRecurrenceOverridestores every DATE-TIMERECURRENCE-IDunder both an ISO key and a date-only key. Commit 3 introduced ISO-key-first with a date-only fallback, but that fallback still applies one instance's override to another when both share a calendar date (e.g.BYHOUR=9,15).Fix: timed events use the ISO key only — a miss unambiguously means "no override for this instance" because every properly stored DATE-TIME RECURRENCE-ID carries both keys.
Regression tests added for both bugs (EXDATE + RECURRENCE-ID) in
test/expand-recurring-event.test.js.Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores