Google Calendar improvements to New or Updated Event source #14925
Conversation
…handle invalid sync token * Bug fix: When the listEvents API returned a 410 status, this threw an exception instead of returning to code that handles this condition. The API call is changed to allow a 410 status to be returned.
… calls when many calendars subscribed * When a notification is received, updates are fetched only for the calendar that triggered the notification This reduces calls to the Google Calendar API, preventing unnecessary rate limiting
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
@fsv-michael is attempting to deploy a commit to the Pipedreamers Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces modifications to the Google Calendar event source component. The Changes
Possibly related PRs
Suggested labels
Poem
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 (
|
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
![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 (2)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (2)
258-259: Improve code style and readability.The ternary expression and array declaration can be more readable with proper line breaks.
- const checkCalendarIds = calendarId ? [ calendarId ] : this.calendarIds; + const checkCalendarIds = calendarId + ? [calendarId] + : this.calendarIds;🧰 Tools
🪛 eslint
[error] 258-258: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 258-258: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 258-258: A linebreak is required after '['.
(array-bracket-newline)
[error] 258-258: A linebreak is required before ']'.
(array-bracket-newline)
Line range hint
273-278: Fix incorrect calendar ID reference in sync token handling.There's a bug in the sync token handling code where
this.calendarIdis used instead of the localcalendarIdvariable.if (syncStatus === 410) { console.log("Sync token invalid, resyncing"); - nextSyncToken = await this.googleCalendar.fullSync(this.calendarId); + nextSyncToken = await this.googleCalendar.fullSync(calendarId); break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs(4 hunks)
🧰 Additional context used
🪛 eslint
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs
[error] 204-204: 'channelId' is assigned a value but never used.
(no-unused-vars)
[error] 258-258: Expected newline between test and consequent of ternary expression.
(multiline-ternary)
[error] 258-258: Expected newline between consequent and alternate of ternary expression.
(multiline-ternary)
[error] 258-258: A linebreak is required after '['.
(array-bracket-newline)
[error] 258-258: A linebreak is required before ']'.
(array-bracket-newline)
🔇 Additional comments (2)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (2)
Line range hint 213-232: LGTM! Improved efficiency in calendar processing.
The changes effectively address the PR objective of reducing API calls by:
- Only processing the relevant calendar when a notification is received
- Providing clear error messaging for debugging
- Using early returns for better flow control
231-232: Verify reduction in API calls.
The changes look good for reducing API calls. Let's verify the impact:
Also applies to: 258-259
✅ Verification successful
API call optimization is properly scoped and implemented
The verification shows that the calendar ID check is uniquely important for the webhook-based instant source, as other components either:
- Use direct event/calendar IDs for single-item operations (get/update)
- List all calendars intentionally (list-calendars)
- Handle pagination and sync tokens appropriately (common.mjs)
The changes effectively reduce API calls by:
- Only querying the specific calendar that triggered the webhook
- Using sync tokens with proper validation
- Maintaining separate channel IDs per calendar
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other calendar API calls in the codebase that might need similar optimization
# Look for listEvents calls that might need the validateStatus parameter
ast-grep --pattern 'listEvents({
$$$
})'
# Look for other potential calendar operations that might need optimization
rg -l "googleCalendar\." | xargs rg "\.list|\.get|\.watch"
Length of output: 6138
...onents/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (3)
Line range hint
212-231: Consider adding debug logging for successful channel ID matches.The error case is well-logged, but adding debug logging for successful matches would help with monitoring and debugging.
calendarId = this.getCalendarIdForChannelId(incomingChannelId); +console.log(`Matched incoming channel ID ${incomingChannelId} to calendar ID ${calendarId}`); if (!calendarId) {
274-278: Enhance logging for resync operations.While the sync token handling is correct, consider adding more detailed logging to track the resync operation's progress and outcome.
-console.log("Sync token invalid, resyncing"); +console.log(`Sync token invalid for calendar ${calendarId}, initiating full resync`); nextSyncToken = await this.googleCalendar.fullSync(calendarId); +console.log(`Full resync completed for calendar ${calendarId}, new sync token obtained`);
Line range hint
212-297: Great architectural improvements for scalability!The changes effectively address both objectives:
- Proper handling of invalid sync tokens through status validation and resync
- Significant reduction in API calls by targeting specific calendars
This will greatly improve performance for users with multiple calendars, reducing API calls by a factor proportional to the number of configured calendars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs(4 hunks)
🧰 Additional context used
🪛 eslint
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs
[error] 258-258: A linebreak is required after '['.
(array-bracket-newline)
[error] 258-258: A linebreak is required before ']'.
(array-bracket-newline)
🔇 Additional comments (2)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (2)
202-208: LGTM! Implementation looks clean and efficient.
The method effectively maps channel IDs to calendar IDs, which is crucial for the PR's goal of reducing API calls by only checking relevant calendars.
257-260: LGTM! Efficient calendar selection logic.
The implementation correctly narrows down API calls to only the relevant calendar when possible, which directly addresses the rate limiting concerns.
🧰 Tools
🪛 eslint
[error] 258-258: A linebreak is required after '['.
(array-bracket-newline)
[error] 258-258: A linebreak is required before ']'.
(array-bracket-newline)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (1)
276-280: Consider adding error logging for other status codesWhile the handling of 410 status code is good, consider adding error logging for other non-success status codes that aren't 410. This would help with debugging API issues.
- validateStatus: (status) => (status >= 200 && (status < 300 || status == 410)), + validateStatus: (status) => { + if (status >= 200 && status < 300) return true; + if (status === 410) return true; + console.log(`Unexpected status code: ${status}`); + return false; + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
components/google_calendar/package.json(1 hunks)components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/google_calendar/package.json
🔇 Additional comments (4)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (4)
11-11: LGTM: Version bump is appropriate
Version increment reflects the significant changes in functionality around channel ID handling and sync token validation.
202-208: Previous review comment still applies
The implementation can still be simplified as noted in the previous review.
Line range hint 212-231: LGTM: Improved channel ID verification logic
The new implementation efficiently handles channel ID verification and provides clear error messaging for debugging purposes. The null initialization pattern with early return makes the code flow clear and maintainable.
257-262: LGTM: Efficient calendar filtering
The implementation now correctly filters calendars based on the channel ID, significantly reducing unnecessary API calls when handling notifications for specific calendars.
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (1)
Line range hint
212-231: Consider enhancing the error message for better debuggingThe channel ID verification logic is solid, but the error message could be more actionable.
Consider this enhancement:
- `Unexpected channel ID ${incomingChannelId}. This likely means there are multiple, older subscriptions active.`, + `Unexpected channel ID ${incomingChannelId}. This likely means there are multiple, older subscriptions active. Consider deactivating and reactivating the source to clean up subscriptions.`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs(4 hunks)
🔇 Additional comments (4)
components/google_calendar/sources/new-or-updated-event-instant/new-or-updated-event-instant.mjs (4)
11-11: LGTM: Version bump is appropriate
The version increment to 0.1.14 appropriately reflects the significant changes in functionality.
202-209: LGTM: Clean implementation of calendar ID lookup
The implementation efficiently maps channel IDs to calendar IDs and correctly returns null if no match is found.
257-262: LGTM: Efficient calendar selection logic
The implementation correctly processes either a single calendar or all calendars based on the webhook context, which significantly reduces API calls.
267-299: Consider adding rate limiting and pagination optimizations
While the error handling for sync tokens is robust, consider these improvements:
- Rate Limiting: Add delay between API calls when processing multiple calendars to avoid hitting Google Calendar API limits.
- Pagination: Consider implementing batch size control to manage memory usage with large result sets.
Let's verify the current API usage patterns:
|
Hi everyone, all test cases are passed! Ready for release! |
Attention @michelle0927 who already made improvements to this source to address API rate limiting errors.
This pull request addresses two issues with the Google Calendar "New or Updated Event (Instant)" event source:
Code to handle an invalid sync token was never reached, because when the API call returned a 410 status code an exception was thrown, instead of returning the 410 status.
This issue has been addressed by passing a custom validateStatus function that allows a 410 response to be returned to the code that handles this.
Our application has approximately 30 calendar IDs configured in a single source.
The event handler would poll all configured calendars for updates whenever any notification was received. This would result in rate limiting errors from the Google Calendar API, quickly causing problems for all our workflows that use also Google Calendar APIs.
With the modified code, when a notification is received, the code checks which subscription triggered the notification, and then requests updates only for the relevant calendar ID. In our use case, this reduces calls to Google Calendar APIs by a factor of 30.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores