feat(providers): improve microsoft provider #444
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/providers/src/calendars/microsoft-calendar/events/format.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/events/format.ts:47">
P1: For `ZonedDateTime`, `value.toInstant().toString()` returns a UTC timestamp with 'Z' suffix (e.g., "2024-12-28T10:00:00.000Z"). Microsoft Graph API expects `dateTime` to be a local datetime string without timezone offset when `timeZone` is specified separately. Use `toPlainDateTime()` instead to match the API contract and the parsing logic in parse.ts.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar.ts:427">
P2: Empty catch block swallows all errors from cancel API. If cancel fails for unexpected reasons (network error, auth issue, rate limit), the code silently falls back to delete without notifications. Consider catching only expected errors (e.g., 'cancel endpoint not available for this event type') and re-throwing unexpected errors.</violation>
<violation number="2" location="packages/providers/src/calendars/microsoft-calendar.ts:458">
P1: The `sendUpdate` parameter doesn't actually control attendee notifications. The 'Prefer: return=minimal' header only affects response size, not notification behavior. When `sendUpdate` is true, an empty header value is set which has no effect. Consider documenting this limitation or removing the unused parameter.</violation>
</file>
<file name="packages/providers/src/lib/events.ts">
<violation number="1" location="packages/providers/src/lib/events.ts:4">
P2: Type guard condition is more restrictive than the `Meeting` type definition. The `Meeting` type only requires `attendees` to be present, but this check requires `length > 1`. An event with exactly 1 attendee would satisfy the `Meeting` type but fail this type guard. Consider using `event.attendees.length > 0` or `event.attendees.length >= 1` to match the type definition, or update the `Meeting` type if the business logic truly requires multiple attendees.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/calendars.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/calendars.ts:23">
P2: Semantic change in `readOnly` handling: When `canEdit` is `undefined`/`null`, the old logic (`!calendar.canEdit`) defaulted to read-only (safer), but the new logic (`calendar.canEdit === false`) now defaults to writable. Consider using `calendar.canEdit !== true` to preserve the conservative behavior, or document this intentional change.</violation>
</file>
Reply to cubic to teach it or ask questions. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| return { | ||
| dateTime: value.toInstant().toString(), |
Contributor
There was a problem hiding this comment.
P1: For ZonedDateTime, value.toInstant().toString() returns a UTC timestamp with 'Z' suffix (e.g., "2024-12-28T10:00:00.000Z"). Microsoft Graph API expects dateTime to be a local datetime string without timezone offset when timeZone is specified separately. Use toPlainDateTime() instead to match the API contract and the parsing logic in parse.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/microsoft-calendar/events/format.ts, line 47:
<comment>For `ZonedDateTime`, `value.toInstant().toString()` returns a UTC timestamp with 'Z' suffix (e.g., "2024-12-28T10:00:00.000Z"). Microsoft Graph API expects `dateTime` to be a local datetime string without timezone offset when `timeZone` is specified separately. Use `toPlainDateTime()` instead to match the API contract and the parsing logic in parse.ts.</comment>
<file context>
@@ -0,0 +1,168 @@
+ }
+
+ return {
+ dateTime: value.toInstant().toString(),
+ timeZone:
+ originalTimeZone?.parsed === value.timeZoneId
</file context>
| await this.graphClient | ||
| .api(`${calendarPath(calendarId)}/events/${eventId}/cancel`) | ||
| .post({}); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: Empty catch block swallows all errors from cancel API. If cancel fails for unexpected reasons (network error, auth issue, rate limit), the code silently falls back to delete without notifications. Consider catching only expected errors (e.g., 'cancel endpoint not available for this event type') and re-throwing unexpected errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/microsoft-calendar.ts, line 427:
<comment>Empty catch block swallows all errors from cancel API. If cancel fails for unexpected reasons (network error, auth issue, rate limit), the code silently falls back to delete without notifications. Consider catching only expected errors (e.g., 'cancel endpoint not available for this event type') and re-throwing unexpected errors.</comment>
<file context>
@@ -327,41 +411,57 @@ export class MicrosoftCalendarProvider implements CalendarProvider {
+ await this.graphClient
+ .api(`${calendarPath(calendarId)}/events/${eventId}/cancel`)
+ .post({});
+ } catch {
+ await this.graphClient
+ .api(`${calendarPath(calendarId)}/events/${eventId}`)
</file context>
Suggested change
| } catch { | |
| } catch (error) { | |
| // Only fallback to delete if cancel endpoint is not available for this event | |
| const err = error as { statusCode?: number; code?: string }; | |
| if (err.statusCode !== 400 && err.code !== 'ErrorInvalidRequest') { | |
| throw error; | |
| } |
|
|
||
| await this.graphClient | ||
| .api(`${calendarPath(sourceCalendar.id)}/events/${eventId}`) | ||
| .header("Prefer", sendUpdate ? "" : "return=minimal") |
Contributor
There was a problem hiding this comment.
P1: The sendUpdate parameter doesn't actually control attendee notifications. The 'Prefer: return=minimal' header only affects response size, not notification behavior. When sendUpdate is true, an empty header value is set which has no effect. Consider documenting this limitation or removing the unused parameter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/microsoft-calendar.ts, line 458:
<comment>The `sendUpdate` parameter doesn't actually control attendee notifications. The 'Prefer: return=minimal' header only affects response size, not notification behavior. When `sendUpdate` is true, an empty header value is set which has no effect. Consider documenting this limitation or removing the unused parameter.</comment>
<file context>
@@ -327,41 +411,57 @@ export class MicrosoftCalendarProvider implements CalendarProvider {
+
+ await this.graphClient
+ .api(`${calendarPath(sourceCalendar.id)}/events/${eventId}`)
+ .header("Prefer", sendUpdate ? "" : "return=minimal")
+ .delete();
+
</file context>
| import type { CalendarEvent, Meeting } from "../interfaces/events"; | ||
|
|
||
| export function isMeeting(event: CalendarEvent): event is Meeting { | ||
| return !!event.attendees && event.attendees.length > 1; |
Contributor
There was a problem hiding this comment.
P2: Type guard condition is more restrictive than the Meeting type definition. The Meeting type only requires attendees to be present, but this check requires length > 1. An event with exactly 1 attendee would satisfy the Meeting type but fail this type guard. Consider using event.attendees.length > 0 or event.attendees.length >= 1 to match the type definition, or update the Meeting type if the business logic truly requires multiple attendees.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/lib/events.ts, line 4:
<comment>Type guard condition is more restrictive than the `Meeting` type definition. The `Meeting` type only requires `attendees` to be present, but this check requires `length > 1`. An event with exactly 1 attendee would satisfy the `Meeting` type but fail this type guard. Consider using `event.attendees.length > 0` or `event.attendees.length >= 1` to match the type definition, or update the `Meeting` type if the business logic truly requires multiple attendees.</comment>
<file context>
@@ -0,0 +1,5 @@
+import type { CalendarEvent, Meeting } from "../interfaces/events";
+
+export function isMeeting(event: CalendarEvent): event is Meeting {
+ return !!event.attendees && event.attendees.length > 1;
+}
</file context>
| }, | ||
| color: calendar.hexColor!, | ||
| readOnly: !calendar.canEdit, | ||
| readOnly: calendar.canEdit === false, |
Contributor
There was a problem hiding this comment.
P2: Semantic change in readOnly handling: When canEdit is undefined/null, the old logic (!calendar.canEdit) defaulted to read-only (safer), but the new logic (calendar.canEdit === false) now defaults to writable. Consider using calendar.canEdit !== true to preserve the conservative behavior, or document this intentional change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/microsoft-calendar/calendars.ts, line 23:
<comment>Semantic change in `readOnly` handling: When `canEdit` is `undefined`/`null`, the old logic (`!calendar.canEdit`) defaulted to read-only (safer), but the new logic (`calendar.canEdit === false`) now defaults to writable. Consider using `calendar.canEdit !== true` to preserve the conservative behavior, or document this intentional change.</comment>
<file context>
@@ -2,31 +2,25 @@ import type { Calendar as MicrosoftCalendar } from "@microsoft/microsoft-graph-t
},
color: calendar.hexColor!,
- readOnly: !calendar.canEdit,
+ readOnly: calendar.canEdit === false,
syncToken: null,
};
</file context>
Suggested change
| readOnly: calendar.canEdit === false, | |
| readOnly: calendar.canEdit !== true, |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Briefly describe what you did and why.
Screenshots / Recordings
Add screenshots or recordings here to help reviewers understand your changes.
Type of Change
Related Areas
Testing
Checklist
Notes
(Optional) Add anything else you'd like to share.
By submitting, I confirm I understand and stand behind this code. If AI was used, I’ve reviewed and verified everything myself.
Summary by cubic
Improves the Microsoft calendar provider with full recurrence support, more reliable delta sync, and better event operations. Also refactors parsing/formatting to handle Microsoft Graph quirks more accurately.
New Features
Bug Fixes
Written for commit fa60b9e. Summary will update automatically on new commits.