refactor(rrule): centralize Temporal rule creation#529
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRecurrence rule construction is centralized in the exported ChangesTemporal rule construction
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
lib/ical-parser-utils.js (2)
579-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the variable name in the error message.
The parameter is named
entry, but the error message referencescurr(likely a leftover from when this logic was inline inical.js).🔨 Proposed fix
- throw new Error('No toISOString function in curr.start ' + entry.start); + throw new Error('No toISOString function in entry.start ' + entry.start);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ical-parser-utils.js` at line 579, Update the error message in the surrounding parser logic to reference the existing entry.start value instead of curr.start, keeping the “No toISOString function” error behavior unchanged.
582-584: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
ensureRruleHasDtstartcall.This block is effectively dead code. The
buildRruleStringForTemporalfunction invoked on line 586 explicitly filters out any segments starting withDTSTART,VALUE=, orTZID=. As a result, theDTSTARTsegment appended byensureRruleHasDtstartis immediately discarded.Unless this call is relied upon for hidden side effects, it can be safely removed.
♻️ Proposed refactor
- if (entry.start.dateOnly) { - rule = ensureRruleHasDtstart(rule, entry, tzUtil); - } -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ical-parser-utils.js` around lines 582 - 584, Remove the ensureRruleHasDtstart call and its surrounding entry.start.dateOnly conditional from the rule-building flow before buildRruleStringForTemporal, leaving the existing rule processing unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/ical-parser-utils.js`:
- Line 586: Update the regular expression in the rruleOnly construction to
remove the unsupported v flag and use the g flag, ensuring every three-digit
millisecond fragment is removed without causing load-time failures in older
Node.js environments.
---
Nitpick comments:
In `@lib/ical-parser-utils.js`:
- Line 579: Update the error message in the surrounding parser logic to
reference the existing entry.start value instead of curr.start, keeping the “No
toISOString function” error behavior unchanged.
- Around line 582-584: Remove the ensureRruleHasDtstart call and its surrounding
entry.start.dateOnly conditional from the rule-building flow before
buildRruleStringForTemporal, leaving the existing rule processing unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 156db305-5342-4da1-9a1d-fb3097246982
📒 Files selected for processing (2)
ical.jslib/ical-parser-utils.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/regression-fixes.test.js (1)
515-517: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCheck all time components to strictly assert exact local midnight.
The test description specifies "exact local midnight", and
event.startis thoroughly checked for minutes, seconds, and milliseconds. However, the loop only checks that the occurrence's hour is0. Consider checking all time components to ensure no fractional offsets are introduced.♻️ Proposed fix
for (const occurrence of occurrences) { assert.equal(occurrence.getHours(), 0, `occurrence ${occurrence.toISOString()} should stay at local midnight`); + assert.equal(occurrence.getMinutes(), 0); + assert.equal(occurrence.getSeconds(), 0); + assert.equal(occurrence.getMilliseconds(), 0); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/regression-fixes.test.js` around lines 515 - 517, Update the occurrence assertions in the regression test loop to verify exact local midnight: assert hours are 0 and also minutes, seconds, and milliseconds are 0, matching the existing event.start checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/regression-fixes.test.js`:
- Around line 515-517: Update the occurrence assertions in the regression test
loop to verify exact local midnight: assert hours are 0 and also minutes,
seconds, and milliseconds are 0, matching the existing event.start checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12cb4e47-4508-4b0f-8cce-bf483d4528b5
📒 Files selected for processing (2)
lib/ical-parser-utils.jstest/regression-fixes.test.js
47f7fbe to
d01e4b2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
I noticed the RRULE building logic was spread across three chained calls at the use site (DTSTART embedding, rule string prep, Temporal wrapper), so I pulled it into one
createTemporalRule()function to make it easier to follow. Export surface shrinks from six internal helpers to one.Summary by CodeRabbit
DTSTART;VALUE=DATE, including when expanding occurrences.DTSTART;VALUE=DATE, verifying each expanded occurrence retains zeroed time components.