update to detect duplicate DTSTART/DTEND in same ics event, #348
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded@jens-maus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 46 seconds before requesting another review. ⌛ 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. WalkthroughThe changes in this pull request involve significant updates to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (4)
README.md (1)
Line range hint
1-1: Add documentation for duplicate DTSTART/DTEND handling.The PR implements error handling for duplicate DTSTART/DTEND entries in ICS events, but this new validation isn't documented. Users should be informed about these potential errors and how to handle them.
Please add a section about error handling that includes these new validations. For example:
+### Error Handling + +The library performs validation on ICS events and will throw errors for invalid data: + +- Duplicate DTSTART or DTEND entries in an event + ```javascript + try { + const events = await ical.async.parseFile('calendar.ics'); + } catch (error) { + if (error.message.includes('duplicate DTSTART') || + error.message.includes('duplicate DTEND')) { + console.error('Invalid calendar data:', error.message); + // Handle the duplicate date entries + } + } + ```🧰 Tools
🪛 LanguageTool
[grammar] ~140-~140: An auxiliary verb seems to be missing from this progressive structure. Did you mean “you're using”, “you are using”, or “you were using”?
Context: ... { console.log(data); }); ``` note: If you using the ical.async.xxx functions is a separ...(PRP_VBG)
[typographical] ~141-~141: Two consecutive dots
Context: ...will be posted to that separate function.. so try/catch must be used around the ic...(DOUBLE_PUNCTUATION)
ical.js (3)
648-663: LGTM! Consider enhancing the error message.The duplicate detection for DTSTART and DTEND is well implemented. However, the error message could be more helpful by including both the existing and duplicate values.
Consider this enhancement:
- throw new Error('duplicate DTSTART encountered, line=' + line); + throw new Error(`duplicate DTSTART encountered at line=${line}, existing value=${curr.start.toISOString()}`);- throw new Error('duplicate DTEND encountered, line=' + line); + throw new Error(`duplicate DTEND encountered at line=${line}, existing value=${curr.end.toISOString()}`);
626-629: Consider using TypeError for consistency.For consistency with other error handling in the file, consider using TypeError when the toISOString function is missing.
Apply this change:
- throw new Error('No toISOString function in curr.start ' + curr.start); + throw new TypeError('No toISOString function in curr.start ' + curr.start);
375-375: Consider centralizing date validation.The codebase has multiple locations checking for
toISOStringfunction existence. Consider creating a utility function to standardize this validation:function validateDateObject(date, context) { if (!date || typeof date.toISOString !== 'function') { throw new TypeError(`Invalid date object in ${context}: ${date}`); } return date; }This would simplify the error handling and ensure consistent validation across the codebase.
Also applies to: 551-551, 626-629
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (1 hunks)
- ical.js (4 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~140-~140: An auxiliary verb seems to be missing from this progressive structure. Did you mean “you're using”, “you are using”, or “you were using”?
Context: ... { console.log(data); }); ``` note: If you using the ical.async.xxx functions is a separ...(PRP_VBG)
[typographical] ~141-~141: Two consecutive dots
Context: ...will be posted to that separate function.. so try/catch must be used around the ic...(DOUBLE_PUNCTUATION)
🔇 Additional comments (1)
ical.js (1)
375-375: LGTM! Improved error handling for date conversion.The enhanced error messages in
exdateParameterandrecurrenceParameternow provide better context when date objects lack thetoISOStringmethod, making debugging easier.Also applies to: 551-551
throw new error
duplicate ?? encountered, line=
?? = DTSTART or DTEND,
line is the ICS file data line contents
for example
or
fixes #172 (bad ics data)
also documents proper way to use try/catch for async node-ical invocation
updates readme about try/catch around async functions, per conversation in #172
Summary by CodeRabbit
Documentation
README.mdregarding thenode-icallibrary's functionality.sync,async, andautodetect.asyncmethods and refined recurrence rules section for better understanding.Bug Fixes
ical.jsfile, providing more informative messages for debugging.