v0.4.0: Lazy recurrence, sweep-line overlaps, benchmarks & CI improvements#4
Merged
AriajSarkar merged 39 commits intomainfrom Mar 12, 2026
Merged
v0.4.0: Lazy recurrence, sweep-line overlaps, benchmarks & CI improvements#4AriajSarkar merged 39 commits intomainfrom
AriajSarkar merged 39 commits intomainfrom
Conversation
…eration Add Recurrence::occurrences() method returning a lazy OccurrenceIterator that computes each occurrence on demand, avoiding upfront allocation. - OccurrenceIterator implements Iterator<Item = DateTime<Tz>> - Skips weekends and exception dates identical to eager path - Supports .take(), .skip(), .nth() and all std iterator combinators - More memory-efficient for large/unbounded recurrence counts - Re-export OccurrenceIterator and Utc from lib.rs public API - Update generate_occurrences doc to recommend lazy API for large counts
…line Rewrites find_overlaps() to use an event sweep-line algorithm, which is significantly faster for calendars with many events. - Replace nested-loop O(N²) approach with checkpoint-based sweep-line - Build (time, is_end, index) checkpoints for each event start/end - Sort checkpoints: ends before starts at equal timestamps (prevents false positives for back-to-back 'touching' events) - Filter zero-duration events before checkpoint construction (start==end events would never enter the active set and could cause edge-case bugs) - Deduplicate pairs via HashSet to avoid reporting same overlap twice - Retain O(1) early-exit for < 2 events after filtering
Add defensive edge-case tests surfaced by CodeRabbit review: src/event.rs: - Assert that EventBuilder rejects zero-duration events (duration_minutes(0) must return Err) tests/gap_validation_tests.rs: - Rework test_zero_duration_events_no_false_overlaps to actually inject a real zero-duration event via Calendar::from_json() (bypasses builder validation, mirrors what untrusted calendar imports can produce) - Confirm find_overlaps returns 0 for a mix of zero-duration + normal events - Add test_touching_events_not_overlapping: three back-to-back 1h events sharing boundaries must produce 0 overlaps tests/property_tests.rs: - Expand property-based tests for recurrence patterns, timezone handling, and gap analysis correctness across randomised inputs
- Add benches/performance.rs with criterion harness covering: overlap detection, gap detection, density analysis, recurrence generation, and is_slot_available (gap + conflict paths) - Allow clippy::unwrap_used at file scope for benchmark harness - Add criterion 0.8 with html_reports to dev-dependencies - Add [[bench]] name='performance' harness=false config - Tidy dependency version specs (drop unnecessary patch versions) - Remove deleted DEVELOPMENT.md/TESTING.md from exclude list - Bump version to 0.3.2
- Add examples/json_web.rs: demonstrates Calendar JSON export/import, event serialization, and web API payload patterns - Add usage-examples/src/json_web.rs: same example for published crate - Wire json_web module into usage-examples/src/main.rs - Add serde_json dependency to usage-examples/Cargo.toml
- Cargo.toml version is the single source of truth - Tag/version mismatch warns but never fails the release - Auto-fix README dependency version before cargo publish - Persist README update to main after successful publish - Cherry-pick README fix to dev branch with conflict handling - Add set -euo pipefail to all shell steps for safety - Use Python tomllib (stdlib 3.11+) for Cargo.toml parsing
Content consolidated into README.md and .github/copilot-instructions.md. These standalone files were redundant and added maintenance burden.
Highlights: lazy recurrence iteration, O(N log N) sweep-line overlap detection, criterion benchmarks, stronger edge-case test coverage, safer zero-duration event handling, release workflow redesign.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds v0.4.0: lazy OccurrenceIterator plus eager generators, full sub‑daily recurrence support (secondly/minutely/hourly) with DST‑aware advancement, streaming occurrences_between with pre‑cap filtering, per‑event caps and JSON/ICS round‑trip for recurrences and exdates, sweep‑line overlap detection, Criterion benches, examples, CI/release workflow updates, and docs cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Consumer as Consumer
participant Recurrence as Recurrence
participant Iterator as OccurrenceIterator
participant FreqHelper as FrequencyHelper
Consumer->>Recurrence: occurrences(start)
Recurrence->>Iterator: create(state)
loop demand-driven
Consumer->>Iterator: next()
Iterator->>FreqHelper: advance_by_frequency(current, freq, interval, intended_time)
alt resolved & valid
Iterator->>Iterator: apply BYDAY / RecurrenceFilter / exdates
alt accepted
Iterator-->>Consumer: Some(DateTime)
else skipped
Iterator-->>Iterator: continue
end
else exhausted/unresolvable
Iterator-->>Consumer: None
end
end
sequenceDiagram
participant EventList as Event List
participant PreFilter as Pre-Filter (active & non-zero)
participant Checkpoints as Checkpoint Builder
participant Sweep as Sweep-Line Processor
participant Active as Active Set
participant Overlaps as Overlap Results
EventList->>PreFilter: provide events
PreFilter->>Checkpoints: build START/END checkpoints
Checkpoints->>Sweep: sorted checkpoints (END before START at same time)
loop process checkpoints
alt END checkpoint
Sweep->>Active: remove event
else START checkpoint
Sweep->>Active: compare with active events
Active-->>Sweep: overlapping candidates
Sweep->>Overlaps: record overlap intervals + titles
Sweep->>Active: add event
end
end
Sweep-->>Overlaps: final overlap list
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
📝 Coding Plan for PR comments
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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: 2
🧹 Nitpick comments (2)
src/recurrence.rs (1)
371-420: Code duplication betweencompute_next()andgenerate_occurrences().The frequency calculation logic is duplicated between the eager
generate_occurrences()method and the lazycompute_next()method. While this is understandable for clarity, it creates a maintenance burden.Consider extracting the date advancement logic into a shared helper method in a future refactor to ensure both paths stay in sync:
fn advance_by_frequency(current: DateTime<Tz>, frequency: Frequency, interval: u16) -> Option<DateTime<Tz>> { // shared logic here }This is optional since both implementations are currently consistent and well-tested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 371 - 420, compute_next() duplicates the date-advancement logic that generate_occurrences() also implements; extract the shared advancement behavior into a single helper (e.g. fn advance_by_frequency(current: DateTime<Tz>, frequency: Frequency, interval: u16) -> Option<DateTime<Tz>>) and have both compute_next() and generate_occurrences() call that helper, moving the Daily/Weekly/Monthly/Yearly branching and timezone-localization steps into advance_by_frequency so logic stays in one place and avoids drift between compute_next and generate_occurrences.src/gap_validation.rs (1)
292-294: Verify the checkpoint sorting order.The sorting uses
b.1.cmp(&a.1)for the secondary key, which should placetrue(END) beforefalse(START) at equal timestamps. Since Rust'sbool::cmpordersfalse < true, reversing the comparison (b.1vsa.1) correctly places END checkpoints first.However, this relies on an implicit ordering of booleans that may not be immediately obvious to readers.
💡 Consider making the ordering more explicit
- // Sort by: (1) time, (2) is_end (true/end comes before false/start) - // This ensures ends are processed before starts at the same timestamp - checkpoints.sort_by(|a, b| a.0.cmp(&b.0).then_with(|| b.1.cmp(&a.1))); + // Sort by: (1) time, (2) checkpoint type (ends before starts at same timestamp) + // This ensures events ending at time T are removed before new events starting at T are added, + // preventing false overlaps for "touching" events. + checkpoints.sort_by(|a, b| { + a.0.cmp(&b.0).then_with(|| { + // is_end=true should come before is_end=false + // Reverse comparison: true > false in standard ordering, so b.1.cmp(&a.1) gives true first + b.1.cmp(&a.1) + }) + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gap_validation.rs` around lines 292 - 294, The comparator for checkpoint sorting is correct but relies on implicit bool ordering; make the intent explicit by changing the secondary comparison in checkpoints.sort_by (the closure comparing a.1 and b.1) to an explicit ordering such as comparing mapped integers or calling cmp().reverse() so that END (true) is always ordered before START (false) at equal timestamps; update the closure used in checkpoints.sort_by to use this explicit comparison to improve readability and avoid relying on implicit bool ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 45-57: The GitHub Actions step that sets PACKAGE_VERSION uses the
bare python command which may not exist on runners; update the
version-resolution command used to set PACKAGE_VERSION (the shell line that
assigns PACKAGE_VERSION via python -c ...) to invoke python3 instead of python
so tomllib is executed with the available interpreter; ensure the rest of the
command (reading Cargo.toml and printing ['package']['version']) remains
unchanged.
In `@usage-examples/src/json_web.rs`:
- Around line 57-69: The example exports a recurring event but Calendar::to_json
and Calendar::from_json currently drop Event.recurrence (see Calendar::to_json
and Calendar::from_json), so the "Weekly Sync" is silently downgraded; fix the
example by removing recurrence from the sample (use
Event::builder().title("Weekly Sync").start(...).duration_hours(1).build()
without .recurrence(...)) or else update/export the JSON format and implement
recurrence serialization/deserialization in
Calendar::to_json/Calendar::from_json and export_to_web so recurrence is
preserved; reference Event::builder, recurrence, export_to_web,
Calendar::to_json, and Calendar::from_json when making the change.
---
Nitpick comments:
In `@src/gap_validation.rs`:
- Around line 292-294: The comparator for checkpoint sorting is correct but
relies on implicit bool ordering; make the intent explicit by changing the
secondary comparison in checkpoints.sort_by (the closure comparing a.1 and b.1)
to an explicit ordering such as comparing mapped integers or calling
cmp().reverse() so that END (true) is always ordered before START (false) at
equal timestamps; update the closure used in checkpoints.sort_by to use this
explicit comparison to improve readability and avoid relying on implicit bool
ordering.
In `@src/recurrence.rs`:
- Around line 371-420: compute_next() duplicates the date-advancement logic that
generate_occurrences() also implements; extract the shared advancement behavior
into a single helper (e.g. fn advance_by_frequency(current: DateTime<Tz>,
frequency: Frequency, interval: u16) -> Option<DateTime<Tz>>) and have both
compute_next() and generate_occurrences() call that helper, moving the
Daily/Weekly/Monthly/Yearly branching and timezone-localization steps into
advance_by_frequency so logic stays in one place and avoids drift between
compute_next and generate_occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7919521b-46d4-4c6d-8292-3c7e887c3095
📒 Files selected for processing (17)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdCargo.tomlDEVELOPMENT.mdTESTING.mdbenches/performance.rsexamples/json_web.rssrc/event.rssrc/gap_validation.rssrc/lib.rssrc/recurrence.rstests/gap_validation_tests.rstests/property_tests.rsusage-examples/Cargo.tomlusage-examples/src/json_web.rsusage-examples/src/main.rs
💤 Files with no reviewable changes (2)
- TESTING.md
- DEVELOPMENT.md
Deduplicate date-advancement logic between generate_occurrences() and OccurrenceIterator::compute_next(). Both now call the same advance_by_frequency() function, eliminating drift risk between the eager and lazy paths.
Add explicit comment on reversed bool comparison to make the END-before-START ordering intent obvious at a glance.
Ubuntu runners may not have bare 'python' on PATH; python3 is the reliable interpreter that ships with tomllib in stdlib.
Calendar::to_json/from_json does not serialize recurrence rules, so the recurring event was silently downgraded on round-trip. Use a plain single event instead to avoid misleading output.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usage-examples/src/json_web.rs (1)
83-100: Consider collapsing the duplicated IO error handling.Both branches build the same shape of message and return path. A tiny helper keeps the example focused on the JSON workflow and reduces copy/paste maintenance.
♻️ Possible cleanup
- if let Err(e) = std::fs::create_dir_all(&output_dir) { - eprintln!("❌ Failed to create directory '{}': {}", output_dir.display(), e); - return Err(eventix::EventixError::Other(format!( - "Failed to create directory '{}': {}", - output_dir.display(), - e - ))); - } - - match std::fs::write(&output_file, &json) { - Ok(_) => println!("\n💾 Saved to {}", output_file.display()), - Err(e) => { - eprintln!("❌ Failed to write file '{}': {}", output_file.display(), e); - return Err(eventix::EventixError::Other(format!( - "Failed to write file '{}': {}", - output_file.display(), - e - ))); - } - } + let io_error = |action: &str, path: &std::path::Path, e: std::io::Error| { + eprintln!("❌ Failed to {} '{}': {}", action, path.display(), e); + eventix::EventixError::Other(format!( + "Failed to {} '{}': {}", + action, + path.display(), + e + )) + }; + + std::fs::create_dir_all(&output_dir) + .map_err(|e| io_error("create directory", &output_dir, e))?; + + std::fs::write(&output_file, &json) + .map_err(|e| io_error("write file", &output_file, e))?; + + println!("\n💾 Saved to {}", output_file.display());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usage-examples/src/json_web.rs` around lines 83 - 100, The code duplicates IO error logging and returning EventixError::Other for both std::fs::create_dir_all(&output_dir) and std::fs::write(&output_file, &json); extract a small helper (e.g., fn report_io_err<T>(action: &str, path: &std::path::Path, err: &dyn std::error::Error) -> Result<T, eventix::EventixError>) or a closure that does the eprintln! with the same formatted message and returns Err(eventix::EventixError::Other(...)); then replace the two duplicated Err branches to call this helper with appropriate action strings and the relevant output_dir/output_file and error value so the example focuses on JSON flow without repeating logging/return logic.src/recurrence.rs (1)
306-307: Consider documenting unsupported frequencies.The
_ => Nonecatch-all silently fails forFrequency::Secondly,Frequency::Minutely, andFrequency::Hourly. Users might not realize these frequencies are unsupported until the iterator unexpectedly terminates after one occurrence.Consider adding a doc comment to
occurrences()oradvance_by_frequencynoting the supported frequencies (Daily, Weekly, Monthly, Yearly).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 306 - 307, Add documentation clarifying which Frequency variants are supported and that others silently return None; update the doc comment for occurrences() (or advance_by_frequency) to list supported frequencies: Daily, Weekly, Monthly, Yearly, and explicitly mention that Secondly, Minutely, and Hourly are not implemented and will cause the iterator to terminate after the first occurrence. Reference the occurrences() method and the advance_by_frequency function and the Frequency enum variants (Frequency::Daily, ::Weekly, ::Monthly, ::Yearly, ::Secondly, ::Minutely, ::Hourly) in the comment so callers know the limitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/gap_validation.rs`:
- Line 255: Replace the nondeterministic HashSet usage with a deterministic
BTreeSet to preserve stable ordering of emitted EventOverlap items: change the
import from std::collections::HashSet to std::collections::BTreeSet, update any
type annotations and construction sites that use HashSet (e.g., variables named
active_set, active_events, or similar in the gap validation logic and the
functions that produce Vec<EventOverlap>) to BTreeSet::new(), and ensure any
insertion/iteration uses the BTreeSet API so the Vec output order is stable;
apply the same replacement to the other occurrences referenced around the later
block (the second active-set usage).
In `@src/recurrence.rs`:
- Around line 273-305: The Monthly/Yearly branches (Frequency::Monthly and
Frequency::Yearly) currently call with_month / with_year and return None when
the target date is invalid (e.g., Jan 31 → Feb), causing recurrences to
terminate; change this to clamp the day to the last valid day of the target
month/year: compute target_year and target_month as you already do, then attempt
to construct a NaiveDate using the original day and if with_month/with_year
yields None, fall back by trying chrono::NaiveDate::from_ymd_opt(target_year,
target_month, day) and decrement the day until from_ymd_opt returns Some (this
effectively clamps to the month's last day), then build the NaiveDateTime and
convert with current.timezone().from_local_datetime(...).earliest(); apply the
same clamping approach in both Frequency::Monthly and Frequency::Yearly code
paths (referencing current.date_naive(), with_month, with_year, and
current.time()).
---
Nitpick comments:
In `@src/recurrence.rs`:
- Around line 306-307: Add documentation clarifying which Frequency variants are
supported and that others silently return None; update the doc comment for
occurrences() (or advance_by_frequency) to list supported frequencies: Daily,
Weekly, Monthly, Yearly, and explicitly mention that Secondly, Minutely, and
Hourly are not implemented and will cause the iterator to terminate after the
first occurrence. Reference the occurrences() method and the
advance_by_frequency function and the Frequency enum variants (Frequency::Daily,
::Weekly, ::Monthly, ::Yearly, ::Secondly, ::Minutely, ::Hourly) in the comment
so callers know the limitation.
In `@usage-examples/src/json_web.rs`:
- Around line 83-100: The code duplicates IO error logging and returning
EventixError::Other for both std::fs::create_dir_all(&output_dir) and
std::fs::write(&output_file, &json); extract a small helper (e.g., fn
report_io_err<T>(action: &str, path: &std::path::Path, err: &dyn
std::error::Error) -> Result<T, eventix::EventixError>) or a closure that does
the eprintln! with the same formatted message and returns
Err(eventix::EventixError::Other(...)); then replace the two duplicated Err
branches to call this helper with appropriate action strings and the relevant
output_dir/output_file and error value so the example focuses on JSON flow
without repeating logging/return logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6171904f-9097-4235-99a4-f500f2bffaeb
📒 Files selected for processing (5)
.github/workflows/release.ymlexamples/json_web.rssrc/gap_validation.rssrc/recurrence.rsusage-examples/src/json_web.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/json_web.rs
Add tests for paths flagged by Codecov as uncovered: - test_lazy_iterator_until: exercises the 'until' exhaustion path - test_lazy_iterator_size_hint_until: covers size_hint with until (returns None) - test_lazy_iterator_size_hint_infinite: covers size_hint with no bounds - test_lazy_iterator_with_interval: exercises interval > 1 via lazy path
Replace HashSet with BTreeSet for the active-event set in the sweep-line algorithm. HashSet iteration is nondeterministic, which could produce different overlap orderings for identical inputs across runs.
Previously, monthly recurrence starting on day 31 would terminate when the target month had fewer days (e.g. Jan 31 → Feb has no day 31). Similarly, yearly recurrence from Feb 29 would stop in non-leap years. Now clamps to the last valid day of the target month (e.g. Jan 31 → Feb 28, Feb 29 → Feb 28 in non-leap years). This matches standard calendar library behavior. - Extract clamp_day_to_month() helper for both Monthly and Yearly paths - Document supported frequency variants (Daily/Weekly/Monthly/Yearly) and note that Secondly/Minutely/Hourly are not yet supported - Add test_monthly_day_clamping and test_yearly_leap_day_clamping tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/recurrence.rs (1)
198-209:⚠️ Potential issue | 🟠 MajorThe eager cap now silently drops valid occurrences in long-range queries.
count_limittruncates generation before any caller-side date filtering happens, and the current call sites insrc/event.rs:84-115/src/calendar.rs:150-176hardcodemax_occurrences = 1000. That means a long-running daily rule queried across a large window will under-report results with no indication that the vector is partial. Please either drive those paths from the lazy iterator up to the requested end bound, or surface truncation explicitly instead of returning a silently incomplete result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 198 - 209, The generate_occurrences function currently computes count_limit by clamping self.count against the caller-supplied max_occurrences (variable count_limit), which can silently drop valid occurrences for large date ranges; change generate_occurrences to stop using the eager max_occurrences cap for generation and instead either (A) accept an explicit end bound (e.g., add a parameter end: DateTime<Tz>) and generate occurrences lazily up to that end (or return an iterator) so callers in event.rs and calendar.rs can drive generation to the requested end, or (B) if you must keep a max_occurrences parameter, return an explicit truncation indicator (e.g., change Result<Vec<DateTime<Tz>>> to a struct { occurrences: Vec<...>, truncated: bool } or return a specific error) so callers know the vector is partial; locate this logic in generate_occurrences (and references to count_limit) and update event.rs/calendar.rs call sites to use the new signature/iterator or check the truncation flag.
♻️ Duplicate comments (1)
src/recurrence.rs (1)
273-305:⚠️ Potential issue | 🟠 MajorMonth-end and leap-day recurrences still exhaust early on invalid target dates.
with_month()/with_year()returnNonefor cases like Jan 31 → Feb or Feb 29 → a non-leap year, and thatNonenow stops bothgenerate_occurrences()andOccurrenceIteratorbeforecount()is satisfied. If the intended behavior is calendar-style recurrence, this needs day clamping instead of terminating the series.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 273 - 305, The monthly/yearly branches in the recurrence logic (inside the Frequency::Monthly and Frequency::Yearly handling that use current.date_naive(), with_month(), and with_year()) prematurely return None when the target month/year has no matching day (e.g., Jan 31 → Feb or Feb 29 → non-leap year), which halts generate_occurrences()/OccurrenceIterator; instead, compute the target year and month, determine the last valid day for that target month (e.g., by querying days_in_month for target_year/target_month) and clamp the day to min(current.day(), last_day_of_target_month) before constructing the NaiveDateTime and converting via current.timezone().from_local_datetime(...).earliest(), so the sequence continues using the calendar-clamped day.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/recurrence.rs`:
- Around line 221-223: The occurrence generators currently only call
advance_by_frequency(current, self.frequency, self.interval) and ignore
Recurrence::weekdays (BYDAY), so Recurrence::weekly().weekdays(...) doesn't
expand/filter occurrences; update the generators that use advance_by_frequency
(and/or advance_by_frequency itself) so that when self.frequency ==
Frequency::Weekly and self.weekdays is non-empty you expand each week interval
into occurrences on each weekday in self.weekdays (or advance within the week to
the next matching BYDAYs) rather than producing a single next date; ensure the
same logic is applied in both generation paths referenced (the weekly branch and
the other generator path) so the produced set matches weekdays() and
to_rrule_string() output.
- Around line 265-306: The advance_by_frequency function can get interval == 0
which causes no progress and infinite iteration; add an early guard in
advance_by_frequency to handle interval == 0 (e.g., return None or treat it as
an error) before the match so the iterator stops instead of repeatedly returning
the same timestamp; update the function (reference: advance_by_frequency, and
callers like occurrences iterator) to check if interval == 0 and bail out
(return None) to prevent non-terminating behavior.
---
Outside diff comments:
In `@src/recurrence.rs`:
- Around line 198-209: The generate_occurrences function currently computes
count_limit by clamping self.count against the caller-supplied max_occurrences
(variable count_limit), which can silently drop valid occurrences for large date
ranges; change generate_occurrences to stop using the eager max_occurrences cap
for generation and instead either (A) accept an explicit end bound (e.g., add a
parameter end: DateTime<Tz>) and generate occurrences lazily up to that end (or
return an iterator) so callers in event.rs and calendar.rs can drive generation
to the requested end, or (B) if you must keep a max_occurrences parameter,
return an explicit truncation indicator (e.g., change Result<Vec<DateTime<Tz>>>
to a struct { occurrences: Vec<...>, truncated: bool } or return a specific
error) so callers know the vector is partial; locate this logic in
generate_occurrences (and references to count_limit) and update
event.rs/calendar.rs call sites to use the new signature/iterator or check the
truncation flag.
---
Duplicate comments:
In `@src/recurrence.rs`:
- Around line 273-305: The monthly/yearly branches in the recurrence logic
(inside the Frequency::Monthly and Frequency::Yearly handling that use
current.date_naive(), with_month(), and with_year()) prematurely return None
when the target month/year has no matching day (e.g., Jan 31 → Feb or Feb 29 →
non-leap year), which halts generate_occurrences()/OccurrenceIterator; instead,
compute the target year and month, determine the last valid day for that target
month (e.g., by querying days_in_month for target_year/target_month) and clamp
the day to min(current.day(), last_day_of_target_month) before constructing the
NaiveDateTime and converting via
current.timezone().from_local_datetime(...).earliest(), so the sequence
continues using the calendar-clamped day.
- Return None from advance_by_frequency when interval is 0, preventing infinite loops in both eager and lazy iteration paths - Apply by_weekday filter in generate_occurrences() and OccurrenceIterator::next() so Recurrence::weekdays() actually filters occurrences (previously the builder method was accepted but ignored) - Add test_zero_interval_does_not_loop: confirms both paths terminate - Add test_weekdays_filter_lazy: daily recurrence with Mon/Wed/Fri filter yields only matching weekdays - Add test_weekdays_filter_eager: verifies eager and lazy paths agree
Owner
Author
|
@coderabbitai Skipped - changing the |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
…ces in long-range queries
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/recurrence.rs (1)
215-245:⚠️ Potential issue | 🟠 Major
count()is capping scanned slots, not emitted occurrences.With
weekdays()set, both generators consume the limit before the BYDAY filter, socount(14)can legitimately return only 6 dates. That contradicts thecount()docs and makesto_rrule_string()describe a different rule than the generators execute. Count bookkeeping needs to happen after a candidate survives the weekday filter.Also applies to: 432-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 215 - 245, The current loop uses count_limit to cap scanned candidates (for _ in 0..count_limit) so BYDAY/weekday filtering can skip candidates and consume the cap; change the logic so count bookkeeping happens after a candidate passes filters and is pushed into occurrences: replace the for loop with a loop that continues until occurrences.len() == count_limit (or decrement count_limit only when you push into occurrences), keeping the same weekday check and advance_by_frequency(current, self.frequency, self.interval) logic to advance on both skip and add paths; apply the same change to the other generator block that uses the same pattern (the block that also references count_limit, by_weekday, current, occurrences, and advance_by_frequency).src/gap_validation.rs (1)
250-271:⚠️ Potential issue | 🟠 Major
find_overlaps()still misses conflicts that begin before the query window.This sweep-line is seeded from
calendar.events_between(start, end), butsrc/calendar.rs:150-176/src/event.rs:85-120only return occurrences whose start falls inside the range. An event from 07:30–09:30 is invisible to an 08:00–18:00 query, so overlaps at the left boundary are still dropped.find_overlaps()needs an intersection-based occurrence query here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gap_validation.rs` around lines 250 - 271, find_overlaps currently seeds the sweep with calendar.events_between(start, end) which only returns occurrences whose start is inside the window and thus misses events that begin before the window but overlap it; change the seeding to fetch occurrences that intersect the query interval (i.e., occurrences with occurrence_time < end AND end_time() > start) instead of using events_between — either call an existing intersection-style API (e.g., calendar.events_intersecting or calendar.occurrences_overlapping) or implement that filter in place before the sweep (use occ.occurrence_time and occ.end_time() to test intersection) so the active set includes events that start before the window but overlap it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/recurrence.rs`:
- Around line 305-307: The Daily/Weekly branches in the match on Frequency use
chrono::Duration::days/weeks added to current (a DateTime<Tz>), which shifts
wall-clock time across DST; change Frequency::Daily and Frequency::Weekly to
advance using local date arithmetic instead: convert current to its local
NaiveDate/NaiveTime (as done in the monthly/yearly branches), add days/weeks to
the NaiveDate, then rebuild the DateTime with tz.from_local_datetime() and
handle ambiguous/nonexistent results the same way the monthly/yearly code does;
replace uses of chrono::Duration::days/weeks on DateTime with this
local-preserving approach to keep the same wall-clock hour.
---
Outside diff comments:
In `@src/gap_validation.rs`:
- Around line 250-271: find_overlaps currently seeds the sweep with
calendar.events_between(start, end) which only returns occurrences whose start
is inside the window and thus misses events that begin before the window but
overlap it; change the seeding to fetch occurrences that intersect the query
interval (i.e., occurrences with occurrence_time < end AND end_time() > start)
instead of using events_between — either call an existing intersection-style API
(e.g., calendar.events_intersecting or calendar.occurrences_overlapping) or
implement that filter in place before the sweep (use occ.occurrence_time and
occ.end_time() to test intersection) so the active set includes events that
start before the window but overlap it.
In `@src/recurrence.rs`:
- Around line 215-245: The current loop uses count_limit to cap scanned
candidates (for _ in 0..count_limit) so BYDAY/weekday filtering can skip
candidates and consume the cap; change the logic so count bookkeeping happens
after a candidate passes filters and is pushed into occurrences: replace the for
loop with a loop that continues until occurrences.len() == count_limit (or
decrement count_limit only when you push into occurrences), keeping the same
weekday check and advance_by_frequency(current, self.frequency, self.interval)
logic to advance on both skip and add paths; apply the same change to the other
generator block that uses the same pattern (the block that also references
count_limit, by_weekday, current, occurrences, and advance_by_frequency).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8689d398-729d-42bf-aace-4b5775e9cde1
📒 Files selected for processing (2)
src/gap_validation.rssrc/recurrence.rs
Owner
Author
|
@coderabbitai review all/recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
85-95:⚠️ Potential issue | 🟡 MinorRestore the
Event::builder()call in this example.Line 89 has a stray
useline pasted into the method chain, so this daily recurrence snippet no longer compiles or copy-pastes cleanly.📘 Suggested fix
use eventix::{Duration, Event, Recurrence, timezone}; let tz = timezone::parse_timezone("America/New_York")?; let holiday = timezone::parse_datetime_with_tz("2025-11-27 09:00:00", tz)?; -use eventix::{Duration, Event, Recurrence}; .title("Morning Standup") +let event = Event::builder() + .title("Morning Standup") .start("2025-11-01 09:00:00", "America/New_York") .duration(Duration::minutes(15)) .recurrence(Recurrence::daily().count(30))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 85 - 95, The README example lost the Event::builder() call and has a stray "use eventix::{Duration, Event, Recurrence};" inserted into the method chain; restore the fluent builder by calling Event::builder() before .title(...) and remove the stray use line so the chain reads Event::builder().title(...).start(...).duration(...).recurrence(...).skip_weekends(...).exception_date(...).build()? and references to Duration, Recurrence, timezone parsing remain above the builder example so the snippet compiles and copy-pastes cleanly.
🧹 Nitpick comments (1)
usage-examples/src/json_web.rs (1)
74-74: Consider handling the potential serialization error.While
serde_json::to_string_prettyon aValueconstructed viajson!macro is unlikely to fail, using.unwrap()in example code may encourage bad practices. Consider using?for consistency with the rest of the function's error handling.Suggested fix
let response = api_response(&cal); println!( "\n🌐 API Response:\n{}", - serde_json::to_string_pretty(&response).unwrap() + serde_json::to_string_pretty(&response).map_err(|e| { + eventix::EventixError::Other(format!("JSON serialization error: {}", e)) + })? );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usage-examples/src/json_web.rs` at line 74, Replace the .unwrap() call on serde_json::to_string_pretty(&response) with proper error propagation: have the function return a Result and use the ? operator (i.e., serde_json::to_string_pretty(&response)?), or map the serde_json error into the function's existing error type; update the function signature if needed so that serde_json::Error can be propagated. Ensure you modify the call site where response is serialized and the surrounding function return type to support ?.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 1: The README badge pointing to the old workflow name needs updating
after renaming the workflow to "EventixCI": update the badge URL/alt text in
README.md (line referencing the "Rust CI" badge) to reference the new workflow
name "EventixCI" and its new status path, and also check and update any branch
protection rules or required check names in repository settings that still
reference "Rust CI" so they match "EventixCI".
In @.github/workflows/release.yml:
- Around line 96-99: The conditional on the "Persist README version on main"
step is incorrectly using steps.prepare_readme.outputs.changed (computed from
the tag checkout) to decide whether to run; remove the if: condition that
references steps.prepare_readme.outputs.changed from the step with id
sync_readme_main so the step always runs on tag pushes, and keep the existing
in-step guard (the git diff --quiet README.md check inside the step) to
determine if a commit to main is needed; ensure you reference the step id
sync_readme_main and the README.md diff check when making the change.
In `@CHANGELOG.md`:
- Around line 17-19: Remove the sentence claiming a signature change to
generate_occurrences(start, max_occurrences) → generate_occurrences(start) from
this PR's CHANGELOG entry and restore any wording that only documents
non-breaking behavior; instead add a short note referencing the new helper
generate_occurrences_capped(start, max) if needed, and move the signature-change
wording and examples to the dedicated breaking-change PR; ensure the changelog
only keeps the Count semantics break (Recurrence::count and occurrences_between)
in this release and that generate_occurrences()/generate_occurrences_capped()
references remain correct.
In `@README.md`:
- Around line 131-158: The README sample mixes two Rust examples and has a
duplicate import and a broken fenced code block; fix by removing the duplicate
"use eventix::{Duration, Event, Recurrence};" line, close the first ```rust code
fence after the sub-daily examples (removing the stray opening fence at line
158), then start a new fenced block for the Booking Workflow and include the
correct import "use eventix::{Duration, Event, EventStatus};" before the booking
example so the sub-daily (Recurrence::hourly/minutely/secondly) examples and the
EventStatus booking workflow are separated and compile correctly.
In `@src/recurrence.rs`:
- Around line 378-380: The code currently treats interval==0 as a no-op in
recurrence.rs (returning None) but still serializes/export as a repeating rule
elsewhere; instead reject zero intervals at creation time or make serialization
fail: add a validation in the recurrence constructor/parser (e.g., the
Recurrence::new / from_parts or the function that currently contains the "if
interval == 0 { return None; }" check) to return an Err/Result indicating
invalid input when interval == 0, and remove the silent None; alternatively
ensure to_rrule_string() explicitly returns an error or panics for interval==0
so src/ics.rs cannot export an incorrect RRULE—update any callers/tests to
handle the new error path.
- Around line 606-610: The iterator currently treats Some(vec![]) as a
constraint that never matches, causing infinite loops; change the handling of
self.recurrence.by_weekday so empty vectors are treated as "no constraint"
(equivalent to None) either by normalizing by_weekday to None on
construction/update or by updating the iterator branch: in the block referencing
self.recurrence.by_weekday and result.weekday(), if weekdays.is_empty() treat it
as if there is no weekday filter, otherwise keep the existing contains check and
continue on mismatch; also ensure to_rrule_string() omits the BYDAY= token when
by_weekday is empty (or has been normalized to None) so you don't emit an empty
BYDAY.
- Around line 604-610: The current post-filter that checks
self.recurrence.by_weekday (the block using
weekdays.contains(&result.weekday())) is invalid for Monthly/Yearly rules;
modify the iterator logic so that for Freq::Monthly and Freq::Yearly you expand
BYDAY into all matching weekdays within each month/year interval (reusing the
weekly BYDAY expansion approach used elsewhere) and generate occurrences from
those expanded candidate dates instead of filtering a single day-of-month;
update the code paths around self.recurrence.by_weekday and result.weekday() to
build candidates per-period (or, if you prefer, make the builder reject
weekdays() for non-weekly frequencies and return an error) so the iterator’s
output matches to_rrule_string()’s BYDAY semantics.
---
Outside diff comments:
In `@README.md`:
- Around line 85-95: The README example lost the Event::builder() call and has a
stray "use eventix::{Duration, Event, Recurrence};" inserted into the method
chain; restore the fluent builder by calling Event::builder() before .title(...)
and remove the stray use line so the chain reads
Event::builder().title(...).start(...).duration(...).recurrence(...).skip_weekends(...).exception_date(...).build()?
and references to Duration, Recurrence, timezone parsing remain above the
builder example so the snippet compiles and copy-pastes cleanly.
---
Nitpick comments:
In `@usage-examples/src/json_web.rs`:
- Line 74: Replace the .unwrap() call on serde_json::to_string_pretty(&response)
with proper error propagation: have the function return a Result and use the ?
operator (i.e., serde_json::to_string_pretty(&response)?), or map the serde_json
error into the function's existing error type; update the function signature if
needed so that serde_json::Error can be propagated. Ensure you modify the call
site where response is serialized and the surrounding function return type to
support ?.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9b53a00-d9b8-4f3b-9faf-814003e1e229
📒 Files selected for processing (20)
.github/workflows/ci.yml.github/workflows/release.ymlCHANGELOG.mdCargo.tomlDEVELOPMENT.mdREADME.mdTESTING.mdTODO.mdbenches/performance.rsexamples/json_web.rssrc/event.rssrc/gap_validation.rssrc/lib.rssrc/recurrence.rstests/gap_validation_tests.rstests/property_tests.rsusage-examples/Cargo.tomlusage-examples/src/json_web.rsusage-examples/src/main.rsusage-examples/src/v040_features.rs
💤 Files with no reviewable changes (2)
- DEVELOPMENT.md
- TESTING.md
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
AriajSarkar
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
README.md (1)
80-243: Consider adding an OccurrenceIterator example.The PR introduces a lazy
OccurrenceIteratoras a major new feature (viaRecurrence::occurrences(start)), but the README doesn't demonstrate its usage. Since this iterator supports standard combinators (.take(),.filter(), etc.), a brief example would help users understand this capability.📘 Suggested OccurrenceIterator example
Consider adding a new subsection after line 157:
### Lazy Occurrence Iteration ```rust use eventix::{Duration, Event, OccurrenceIterator, Recurrence, timezone}; let tz = timezone::parse_timezone("UTC")?; let start = timezone::parse_datetime_with_tz("2025-11-01 09:00:00", tz)?; // Create a daily recurrence let pattern = Recurrence::daily().count(30); // Lazily iterate over occurrences let occurrences: Vec<_> = pattern .occurrences(start) .take(10) .collect(); // Use iterator combinators let weekday_occurrences: Vec<_> = pattern .occurrences(start) .filter(|dt| dt.weekday().number_from_monday() <= 5) .take(5) .collect();</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 80 - 243, Summary: README lacks an example for the
new lazy OccurrenceIterator produced by Recurrence::occurrences(start). Add a
short "Lazy Occurrence Iteration" subsection in the Examples that demonstrates
creating a Recurrence, obtaining an OccurrenceIterator via
Recurrence::occurrences(start), and using standard iterator combinators
(.take(), .filter(), .collect()) to produce a Vec of datetimes; reference
types/functions like Recurrence, OccurrenceIterator, timezone::parse_timezone
and timezone::parse_datetime_with_tz so readers can copy the example and run it.</details> </blockquote></details> <details> <summary>src/recurrence.rs (1)</summary><blockquote> `1938-1957`: **Move the 100ms check out of the unit test.** This assertion will be sensitive to runner load and build profile, so it can flap even when the skip logic is correct. The new Criterion benchmarks are a better place to enforce performance expectations; keep this test functional. <details> <summary>♻️ Minimal change</summary> ```diff - let start_time = std::time::Instant::now(); let occurrences: Vec<_> = recurrence.occurrences(start).collect(); - let elapsed = start_time.elapsed(); @@ - assert!( - elapsed.as_millis() < 100, - "subdaily weekday skip took too long: {:?} — O(1) skip may not be working", - elapsed - );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/recurrence.rs` around lines 1938 - 1957, The test measures runtime and asserts elapsed.as_millis() < 100 which can flake; remove the timing measurement and the assert and keep only the functional checks: drop the start_time/elapsed variables and the final assert that references "subdaily weekday skip took too long", leaving the collection of recurrence.occurrences(start), the length check (occurrences.len() == 10) and the weekday/day/month assertions intact; if you need to enforce performance create a Criterion benchmark instead for recurrence.occurrences to validate the skip optimization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 96-109: The workflow step id sync_readme_main can fail if
README.md was modified earlier in the job; before running git checkout main and
git pull, discard or stash the working-change to README.md so the branch switch
succeeds, e.g. run a command to restore/reset the file (restore or checkout --
README.md or stash push/pop) and then perform the sed version bump on main (the
sed line that updates eventix = "$VERSION" should remain after the restore).
Ensure the restore/stash is added immediately before git checkout main in the
Persist README version on main step.
- Around line 133-163: The script treats any non-zero exit from git cherry-pick
as a conflict; instead detect the empty-cherry-pick case and treat it as
“already synced.” After running git cherry-pick "$COMMIT_SHA" capture its
stderr/output and if it contains the empty-pick message (e.g. "The previous
cherry-pick is now empty" or similar), run git cherry-pick --quit (or --abort if
appropriate) and exit 0 with a message that the change is already present;
otherwise keep the existing conflict handling (git cherry-pick --abort, git
reset --hard, echo manual resolution, exit 1). Ensure you update the logic
around the git cherry-pick invocation in the release workflow block that echoes
"🍒 Cherry-picking commit $COMMIT_SHA..." to handle this special-case.
- Around line 25-29: The workflow's test and lint steps run `cargo test
--all-features --verbose` and `cargo clippy --all-features -- -D warnings`,
which omit examples and benches; update those commands to include
`--all-targets` (i.e., run `cargo test --all-features --all-targets --verbose`
and `cargo clippy --all-features --all-targets -- -D warnings`) so
`examples/json_web.rs` and `benches/performance.rs` are compiled and checked as
part of the release gate.
In `@src/recurrence.rs`:
- Around line 812-839: The iterator is being marked exhausted if the first
expanded period yields no candidates (due to start cut-off) because the empty
check occurs before advancing to the next period; to fix, defer the exhausted
check until after advancing the period so Monthly/Yearly cases update
byday_next_month/byday_next_year first (referencing pending_byday, byday_first,
byday_next_month, byday_next_year, recurrence.frequency, Frequency::Monthly,
Frequency::Yearly, and exhausted), i.e., move the if
self.pending_byday.is_empty() { self.exhausted = true } block to after the match
that advances the next period so a subsequent period gets expanded before
declaring exhaustion.
---
Nitpick comments:
In `@README.md`:
- Around line 80-243: Summary: README lacks an example for the new lazy
OccurrenceIterator produced by Recurrence::occurrences(start). Add a short "Lazy
Occurrence Iteration" subsection in the Examples that demonstrates creating a
Recurrence, obtaining an OccurrenceIterator via Recurrence::occurrences(start),
and using standard iterator combinators (.take(), .filter(), .collect()) to
produce a Vec of datetimes; reference types/functions like Recurrence,
OccurrenceIterator, timezone::parse_timezone and
timezone::parse_datetime_with_tz so readers can copy the example and run it.
In `@src/recurrence.rs`:
- Around line 1938-1957: The test measures runtime and asserts
elapsed.as_millis() < 100 which can flake; remove the timing measurement and the
assert and keep only the functional checks: drop the start_time/elapsed
variables and the final assert that references "subdaily weekday skip took too
long", leaving the collection of recurrence.occurrences(start), the length check
(occurrences.len() == 10) and the weekday/day/month assertions intact; if you
need to enforce performance create a Criterion benchmark instead for
recurrence.occurrences to validate the skip optimization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0055b628-bc1a-46f9-96ee-f4922f5ad698
📒 Files selected for processing (4)
.github/workflows/release.ymlREADME.mdsrc/recurrence.rsusage-examples/src/json_web.rs
- Updated `events_between` to use a capped limit for occurrences. - Added `events_between_capped` method to limit occurrences per event. - Improved JSON serialization for recurrence rules and exception dates. - Enhanced `OccurrenceIterator` for better performance with weekday filters. - Added tests for recurrence JSON roundtrip and exception date precision. - Documented lazy occurrence iteration in README.
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/calendar.rs (1)
155-180: Consider surfacing whenevents_between()hits its default per-event cap.The explicit
_cappedAPI is great, butevents_between()still returns a plainVecafter delegating to a 100,000-per-event default. Dense recurrences can therefore truncate silently, and callers cannot tell whether the result is complete or capped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/calendar.rs` around lines 155 - 180, events_between currently hides whether the 100,000-per-event default truncated results; change the API so callers can detect truncation: introduce a small result wrapper (e.g., EventsQueryResult) holding Vec<EventOccurrence<'_>> plus a boolean (or per-event caps map) called e.g. truncated or any_event_capped, update events_between_capped to accumulate that flag when calling Event::occurrences_between (modify occurrences_between to return both occurrences and a capped flag or separate count) and have events_between return the wrapper instead of plain Vec; ensure symbols to change include events_between, events_between_capped, occurrences_between, and EventOccurrence so callers can check the truncated indicator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/calendar.rs`:
- Around line 334-351: The import currently swallows recurrence parse failures
(json_to_recurrence(...).ok()) and quietly ignores malformed EXDATEs, producing
silently altered events; change the JSON import to propagate and surface parsing
errors instead of dropping them: replace json_to_recurrence(...).ok() with
proper Result handling (use ? or map_err and return a descriptive Err) so
invalid recurrence data fails the import, and validate/collect EXDATE parse
results—do not filter out failures silently (collect parsed dates and return an
error if any EXDATE fails to parse). Update the surrounding import function to
return a Result and apply the same fix for the other occurrence between lines
443–489.
In `@src/ics.rs`:
- Around line 223-232: The loop over ical_event.properties() that calls
parse_rrule_value(rrule_value, start_time) must fail the import when
parse_rrule_value reports unsupported pieces instead of silently ignoring Err or
partial support; change the logic around parse_rrule_value so that any Err or a
returned marker of partial/unsupported fields causes the function handling this
ICS to return an error (or otherwise reject the RRULE) rather than calling
builder.recurrence(...) with a degraded recurrence; apply the same change to the
other RRULE-handling blocks that call parse_rrule_value (the later block that
currently modifies builder.recurrence) so unsupported RRULEs are surfaced and
rejected consistently.
- Around line 178-187: The EXDATE branch currently formats each exdate in its
own timezone but always tags it with the event TZID, causing the local clock
time to mismatch; before formatting and adding the EXDATE property (inside the
loop over event.exdates), convert/normalize the exdate to the event's timezone
(e.g. use exdate.with_timezone(&event.timezone) or equivalent) and then format
that normalized datetime into exdate_str, then create the
Property::new("EXDATE", &exdate_str) and add TZID as before so the stamped local
time matches the event TZID; keep the UTC branch unchanged.
In `@src/recurrence.rs`:
- Around line 561-580: skip_subdaily_to_matching_day always advances to the next
day before checking weekdays, which skips a valid sub-daily slot when
compute_next() already landed on an allowed weekday; update
skip_subdaily_to_matching_day to first check if current.date_naive().weekday()
is in weekdays and return current immediately if so, otherwise proceed with the
existing loop that advances target_date (ensure you still use the existing
timezone handling and succ_opt() logic); look for the function
skip_subdaily_to_matching_day and adjust the initial search so the current day
is considered before iterating to subsequent days.
---
Nitpick comments:
In `@src/calendar.rs`:
- Around line 155-180: events_between currently hides whether the
100,000-per-event default truncated results; change the API so callers can
detect truncation: introduce a small result wrapper (e.g., EventsQueryResult)
holding Vec<EventOccurrence<'_>> plus a boolean (or per-event caps map) called
e.g. truncated or any_event_capped, update events_between_capped to accumulate
that flag when calling Event::occurrences_between (modify occurrences_between to
return both occurrences and a capped flag or separate count) and have
events_between return the wrapper instead of plain Vec; ensure symbols to change
include events_between, events_between_capped, occurrences_between, and
EventOccurrence so callers can check the truncated indicator.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f21f3681-eb51-4823-adee-6919180b6680
📒 Files selected for processing (7)
.github/workflows/release.ymlREADME.mdsrc/calendar.rssrc/event.rssrc/ics.rssrc/recurrence.rsusage-examples/src/json_web.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/calendar.rs (1)
461-464: Potential truncation when casting count from u64 to u32.At Line 464,
c as u32will silently truncate if the JSON contains a count value larger thanu32::MAX(4,294,967,295). While unlikely in practice, this could cause unexpected behavior.♻️ Suggested fix using try_into with error handling
if let Some(c) = val["count"].as_u64() { - rec = rec.count(c as u32); + let count: u32 = c.try_into().map_err(|_| { + EventixError::Other(format!("Recurrence count {} exceeds u32::MAX", c)) + })?; + rec = rec.count(count); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/calendar.rs` around lines 461 - 464, The code casts JSON count (c: u64) to u32 with `c as u32`, which can silently truncate large values; update the handling in the Recurrence creation (function using Recurrence::new and rec.count) to perform a fallible conversion (e.g., use u32::try_from or c.try_into()) and propagate or return an error when the value exceeds u32::MAX instead of truncating; ensure the call site (where `rec = rec.count(...)` is assigned) handles the Result/Option accordingly and includes a clear error message referencing the offending `count` value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/calendar.rs`:
- Around line 155-163: The public events_between() currently calls
events_between_capped(..., 100_000) and silently truncates per-event
occurrences; update events_between's docstring to explicitly state that a
100_000 per-event occurrence cap is applied and truncation may occur, and modify
events_between to log a warning (e.g., via the same logger used in this module)
when events_between_capped returns a result that hit the cap so callers are
alerted; additionally update the four callers in gap_validation.rs (find_gaps,
find_overlaps, calculate_density, is_slot_available) to either call
events_between_capped(...) with an explicit cap or handle/propagate the logged
truncation (or switch to the capped API if they require completeness).
In `@src/ics.rs`:
- Around line 240-270: The EXDATE parsing currently swallows parse errors;
update the EXDATE loop to propagate failures like RRULE parsing does instead of
silently skipping: replace the if let Ok(exdate_dt) =
parse_ical_datetime_value(...) branch with code that maps
parse_ical_datetime_value's error into a descriptive Err (or uses ? to bubble it
up) so the surrounding function returns an error when an EXDATE fails to parse;
if you must keep non-fatal behavior, at minimum log a warning with the EXDATE
value and error before continuing. Make sure to reference
parse_ical_datetime_value and builder.exception_date when changing the branch so
errors include the EXDATE string and context.
- Around line 327-346: The parser currently silently skips ordinal-prefixed
BYDAY tokens (e.g., "1MO", "-1FR") in the BYDAY match block, which leads to
incorrect broader schedules; update the BYDAY handling (the loop over
value.split and the match that maps "MO"/"TU"/... to chrono::Weekday and assigns
by_weekday) to detect ordinal prefixes (a leading signed integer before the
weekday) and return an explicit error instead of continuing—mirroring how
BYSETPOS is rejected—so that encountering tokens like "1MO" or "-1FR" yields a
clear Err variant with a descriptive message rather than silently ignoring them.
---
Nitpick comments:
In `@src/calendar.rs`:
- Around line 461-464: The code casts JSON count (c: u64) to u32 with `c as
u32`, which can silently truncate large values; update the handling in the
Recurrence creation (function using Recurrence::new and rec.count) to perform a
fallible conversion (e.g., use u32::try_from or c.try_into()) and propagate or
return an error when the value exceeds u32::MAX instead of truncating; ensure
the call site (where `rec = rec.count(...)` is assigned) handles the
Result/Option accordingly and includes a clear error message referencing the
offending `count` value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52b70323-e647-428e-8529-4bd94f2c019a
📒 Files selected for processing (3)
src/calendar.rssrc/ics.rssrc/recurrence.rs
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/calendar.rs (1)
452-501: Consider validating COUNT+UNTIL mutual exclusion here as well.Similar to the ICS parser, this function allows both
countanduntilto be set simultaneously, which violates RFC 5545 semantics. For consistency, consider adding the same validation here.🛠️ Suggested fix
if let Some(until_str) = val["until"].as_str() { let parsed = chrono::DateTime::parse_from_rfc3339(until_str) .map_err(|e| EventixError::DateTimeParse(format!("recurrence until: {}", e)))?; rec = rec.until(parsed.with_timezone(&tz)); } + + // RFC 5545: COUNT and UNTIL are mutually exclusive + if rec.get_count().is_some() && rec.get_until().is_some() { + return Err(EventixError::Other( + "Recurrence cannot have both 'count' and 'until'".to_string(), + )); + } + if let Some(weekdays_arr) = val["weekdays"].as_array() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/calendar.rs` around lines 452 - 501, In json_to_recurrence ensure COUNT and UNTIL are mutually exclusive by detecting if val["count"] and val["until"] are both present (as a u64/as_str) before applying them and return an EventixError::Other (e.g. "Recurrence cannot have both 'count' and 'until'") if both are set; update the logic around Recurrence::count and Recurrence::until to first validate exclusivity, then proceed to parse and set count (u32 conversion) or parse UNTIL (parse_from_rfc3339 and with_timezone) as currently implemented.src/ics.rs (1)
246-263: Fragile TZID extraction via Debug format parsing.The code extracts TZID values by formatting the parameter with
{:?}and then parsing the resulting string forval: ". This is brittle—if theicalendarcrate changes itsDebugimplementation, this will silently fail and fall back toevent_tz.Consider using a more robust approach if the library exposes a direct accessor, or at minimum add a test that asserts the expected Debug format to catch breakage early.
icalendar rust crate Parameter value accessor method🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ics.rs` around lines 246 - 263, The TZID extraction is brittle because it parses the Debug output of the parameter; replace the fragile string parsing in the exdate_tz block with a proper Parameter value accessor from the icalendar API (use the actual method on the value returned by prop.params().get("TZID") — e.g., call the crate's Parameter::value()/get_value()/to_string() or other documented accessor instead of format!("{:?}")), then feed that string into crate::timezone::parse_timezone(...). Keep the existing fallbacks to event_tz/UTC, and add a unit test around exdate handling (including a TZID parameter case) to detect regressions if the icalendar crate changes its types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ics.rs`:
- Around line 362-375: The parser currently allows both COUNT and UNTIL to be
set; add a validation before building the Recurrence to reject this per RFC 5545
by returning an error when both are present: check if count.is_some() &&
until.is_some() and return Err(EventixError::IcsError("RRULE must not contain
both COUNT and UNTIL".to_string())), otherwise proceed to construct the
Recurrence with Recurrence::new(freq).interval(interval) and apply .count(c) or
.until(u) as before; reference variables: frequency, count, until and functions
Recurrence::new, recurrence.count, recurrence.until.
---
Nitpick comments:
In `@src/calendar.rs`:
- Around line 452-501: In json_to_recurrence ensure COUNT and UNTIL are mutually
exclusive by detecting if val["count"] and val["until"] are both present (as a
u64/as_str) before applying them and return an EventixError::Other (e.g.
"Recurrence cannot have both 'count' and 'until'") if both are set; update the
logic around Recurrence::count and Recurrence::until to first validate
exclusivity, then proceed to parse and set count (u32 conversion) or parse UNTIL
(parse_from_rfc3339 and with_timezone) as currently implemented.
In `@src/ics.rs`:
- Around line 246-263: The TZID extraction is brittle because it parses the
Debug output of the parameter; replace the fragile string parsing in the
exdate_tz block with a proper Parameter value accessor from the icalendar API
(use the actual method on the value returned by prop.params().get("TZID") —
e.g., call the crate's Parameter::value()/get_value()/to_string() or other
documented accessor instead of format!("{:?}")), then feed that string into
crate::timezone::parse_timezone(...). Keep the existing fallbacks to
event_tz/UTC, and add a unit test around exdate handling (including a TZID
parameter case) to detect regressions if the icalendar crate changes its types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b7bab6d-f2e1-4007-b63b-e9daab42e924
📒 Files selected for processing (2)
src/calendar.rssrc/ics.rs
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/calendar.rs (1)
487-506:⚠️ Potential issue | 🔴 CriticalSame type mismatch:
chrono::Weekdayvsrrule::Weekday.This helper builds a
Vec<chrono::Weekday>at lines 488-501, butRecurrence::weekdays()expectsVec<rrule::Weekday>per the struct definition. This mirrors the issue insrc/ics.rs:321-336.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/calendar.rs` around lines 487 - 506, The code builds a Vec<chrono::Weekday> from weekdays_arr but passes it to Recurrence::weekdays which expects Vec<rrule::Weekday>; change the mapping so the match on wd_str yields rrule::Weekday variants (or convert chrono::Weekday to rrule::Weekday) and push those into the weekdays Vec, then call rec.weekdays(weekdays) with the Vec<rrule::Weekday>; update the match arms where "MO".."SU" are mapped (currently to chrono::Weekday::Mon..Sun) to the corresponding rrule::Weekday::Mon..Sun (or perform an explicit conversion helper function) so the types align with Recurrence::weekdays.
🧹 Nitpick comments (1)
src/calendar.rs (1)
469-474: Consider stricter type-agnostic validation for COUNT+UNTIL mutual exclusion.The current check
val["count"].is_u64() && val["until"].is_string()only catches the "happy path" types. Ifcountis provided as a string (e.g.,"10") oruntilas another type, the validation passes but the values are silently dropped during parsing. While this doesn't violate RFC 5545 (you can't have both applied), it may mask user errors in JSON input.Consider checking for the presence of keys regardless of type:
- if val["count"].is_u64() && val["until"].is_string() { + if !val["count"].is_null() && !val["until"].is_null() {This is a minor robustness improvement rather than a correctness issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/calendar.rs` around lines 469 - 474, The mutual-exclusion check currently only triggers when count is a u64 and until is a string; change it to detect the presence of the keys regardless of their JSON types (e.g., use val.get("count").is_some() and val.get("until").is_some() or val.as_object().and_then(|o| o.contains_key("count"))/contains_key("until")) and return the same EventixError::Other("Recurrence cannot have both 'count' and 'until'".to_string()) when both keys exist so malformed types like "count": "10" or non-string until don't silently bypass the validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/calendar.rs`:
- Around line 487-506: The code builds a Vec<chrono::Weekday> from weekdays_arr
but passes it to Recurrence::weekdays which expects Vec<rrule::Weekday>; change
the mapping so the match on wd_str yields rrule::Weekday variants (or convert
chrono::Weekday to rrule::Weekday) and push those into the weekdays Vec, then
call rec.weekdays(weekdays) with the Vec<rrule::Weekday>; update the match arms
where "MO".."SU" are mapped (currently to chrono::Weekday::Mon..Sun) to the
corresponding rrule::Weekday::Mon..Sun (or perform an explicit conversion helper
function) so the types align with Recurrence::weekdays.
---
Nitpick comments:
In `@src/calendar.rs`:
- Around line 469-474: The mutual-exclusion check currently only triggers when
count is a u64 and until is a string; change it to detect the presence of the
keys regardless of their JSON types (e.g., use val.get("count").is_some() and
val.get("until").is_some() or val.as_object().and_then(|o|
o.contains_key("count"))/contains_key("until")) and return the same
EventixError::Other("Recurrence cannot have both 'count' and
'until'".to_string()) when both keys exist so malformed types like "count": "10"
or non-string until don't silently bypass the validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba5917ac-1c7b-4219-808d-ddafd1280dd7
📒 Files selected for processing (2)
src/calendar.rssrc/ics.rs
…r unbounded recurrences
…idate COUNT/UNTIL presence
Owner
Author
|
@coderabbitai review recent commits |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
This release brings performance improvements, stronger test coverage, a new example, and CI workflow enhancements for the
eventixcrate.Changes
✨ Features
OccurrenceIterator—Recurrence::occurrences(start)returns a lazy iterator that computes dates on demand, avoiding upfront allocation for large/unbounded patterns. Supports.take(),.nth(), and all standard iterator combinators.examples/json_web.rsdemonstrating Calendar JSON export/import and web API payload patterns.⚡ Performance
find_overlaps()replaced the O(N²) nested-loop with a checkpoint-based sweep-line algorithm. Processes END before START at equal timestamps to eliminate false positives on touching events. Filters zero-duration events defensively.benches/performance.rscovers overlap detection, gap analysis, density calculation, recurrence generation, and slot availability (both gap and conflict paths).🧪 Tests
duration_minutes(0); test usesCalendar::from_json()to inject real zero-duration events and confirmsfind_overlapshandles them correctly.🔧 CI / Release Workflow
Cargo.tomlis the single source of truth. Tag mismatches warn but never fail.Cargo.tomlbeforecargo publish, then persisted back tomainand cherry-picked todev.run:blocks useset -euo pipefail.🧹 Housekeeping
DEVELOPMENT.mdandTESTING.md(content consolidated into README and copilot-instructions).OccurrenceIteratorandUtcfrom public API.Commits (8)
featOccurrenceIteratorfor memory-efficient recurrence iterationperffind_overlapstestperffeatjson_webexample for JSON serialization workflowscichoreDEVELOPMENT.mdandTESTING.mddocsTesting
Summary by CodeRabbit
New Features
Bug Fixes / Performance
Breaking Changes
Tests
Documentation
Chores