feat(providers): improve provider structure - #474
Open
JeanMeijer wants to merge 7 commits into
Open
Conversation
|
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.
18 issues found across 98 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/providers/src/calendars/microsoft-calendar/events/format.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/events/format.ts:217">
P1: Recurrence-only edits can silently move an existing series into UTC because the formatter receives the patch metadata rather than the master’s stored `recurrenceTimeZone`; this changes recurrence dates and DST behavior for non-UTC series. Preserve the existing master metadata when resolving a sparse recurrence update and pass that timezone through the formatter.</violation>
</file>
<file name="packages/providers/src/lib/events.ts">
<violation number="1" location="packages/providers/src/lib/events.ts:4">
P2: The `isMeeting` predicate is inconsistent with the `Meeting` type it narrows to. `Meeting` only requires `attendees` to be present, so an event with 0 or 1 attendee is a valid `Meeting` value, yet `isMeeting` returns `false` for it (it only returns `true` when there are 2+ attendees). Any consumer relying on the guard to detect `Meeting`s will miss single-attendee events, and the type semantics ('has attendees') no longer match the runtime check ('has multiple attendees'). Consider aligning them: if a meeting means 'has attendees', use `(event.attendees?.length ?? 0) > 0` (or `event.attendees !== undefined`); if it genuinely means 'more than one attendee', update the `Meeting` type/name and add a comment documenting the stricter rule so the divergence is intentional.</violation>
<violation number="2" location="packages/providers/src/lib/events.ts:4">
P3: Heuristic note: classifying "is a meeting" purely by `attendees?.length > 1` ignores attendee roles/status. An event whose only other attendee is a resource room, or a meeting whose counterpart declined (or only the organizer is listed), will be misclassified either way. Since Attendee carries `type` (required/optional/resource), `organizer`, and `status`, counting only non-resource, required attendees (or checking for a conference) would match the intent more reliably.</violation>
</file>
<file name="packages/microsoft-calendar/src/interfaces.ts">
<violation number="1" location="packages/microsoft-calendar/src/interfaces.ts:230">
P2: Making `start`/`end` required on `Event` (and similarly `start`/`end`/`status` on `ScheduleItem`) now promises these fields are always present, but the same interfaces back GET/list/delta responses that support `select` and can legitimately omit those fields. The added comment acknowledges this '$select' caveat but the type still overpromises: consumers that call e.g. `get` with a `select` that excludes `start` can receive objects where `event.start.dateTime` is `undefined`, and code like `packages/providers/src/calendars/microsoft-calendar/events/parse.ts` dereferences it without a guard. Since these fields are required for create input but only conditionally present on `select`-filtered responses, consider separating the create-input shape from the response shape (or keeping the response `Event` fields optional) so the type doesn't force runtime assumptions that `$select` can violate.</violation>
</file>
<file name="apps/app/src/lib/trpc/query-client.ts">
<violation number="1" location="apps/app/src/lib/trpc/query-client.ts:32">
P3: Removing the `?? "Something went wrong"` fallback means that when an error has no message (e.g. a thrown non-Error value), the console now logs `undefined` instead of the earlier friendly message. Consider keeping a fallback for robustness.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/utils.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/utils.ts:6">
P2: Environments that expose `Intl` as `undefined` or `null` now receive a `TypeError` while evaluating this condition instead of the function's documented/custom availability error. Keeping a safe `globalThis.Intl` check preserves the intended failure path for runtimes or tests without `Intl`.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/events/index.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/events/index.ts:475">
P1: A failed cancellation can unexpectedly delete the event: every API 4xx except 401/408/429 falls through to the delete request. Permission, conflict, validation, or other client errors should surface unless the response is specifically the known non-organizer case, otherwise a cancellation failure can become destructive deletion.</violation>
<violation number="2" location="packages/providers/src/calendars/microsoft-calendar/events/index.ts:494">
P3: The added Microsoft `move` implementation is dead from the application's current entry points: the API explicitly rejects Microsoft moves and its standalone move schema only accepts Google calendars. Either wire Microsoft into the move routing/validation or leave this method unsupported until a caller exists.</violation>
<violation number="3" location="packages/providers/src/calendars/microsoft-calendar/events/index.ts:498">
P2: Microsoft moves silently ignore `sendUpdate: false`, so a caller can request no attendee notifications while the copy-and-delete flow still performs the operation; this is inconsistent with the provider's explicit unsupported-mode checks in `create` and `delete`. The move path should accept the option and reject `false` (or otherwise implement the requested notification behavior) before creating the copy.</violation>
<violation number="4" location="packages/providers/src/calendars/microsoft-calendar/events/index.ts:516">
P1: A failed source deletion leaves the original and newly created destination event in place, so this copy-then-delete move can duplicate events and repeat the duplication on retry. The move should validate the source version before copying and provide cleanup/idempotency or an explicit reconciliation path when deletion fails.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/recurrence/parse.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/recurrence/parse.ts:66">
P1: Graph absoluteMonthly/absoluteYearly rules on dates that do not exist in every target month or year are returned as RFC `BYMONTHDAY` rules, changing which occurrences users see. The parser should preserve this provider-specific behavior or reject these unsupported recurrences instead of exposing a misleading rule.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/events/parse.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/events/parse.ts:139">
P1: Editing a Microsoft online-meeting event can fail input validation because this stores Graph's raw nullable `OnlineMeetingInfo` in metadata, but the update schema only permits omitted or non-null values. Normalize nullable fields to `undefined`/filtered strings before returning metadata so the provider metadata can round-trip through event updates.</violation>
</file>
<file name="packages/providers/src/calendars/google-calendar/events/parse.ts">
<violation number="1" location="packages/providers/src/calendars/google-calendar/events/parse.ts:57">
P1: Timed Google events whose `dateTime` omits an offset and supplies `timeZone` cannot be parsed: `Temporal.Instant.from` throws before the supplied timezone is applied. Parse offset-bearing values as instants, but parse local date-times with `Temporal.PlainDateTime.from(...).toZonedDateTime(...)`.</violation>
<violation number="2" location="packages/providers/src/calendars/google-calendar/events/parse.ts:253">
P2: An attendee response without `email` is emitted with `email: undefined`, violating the provider `Attendee` contract and its required email schema. Skip or otherwise explicitly handle attendees lacking an email before constructing the normalized attendee list rather than relying on `!`.</violation>
</file>
<file name="packages/providers/src/calendars/google-calendar/events/format.ts">
<violation number="1" location="packages/providers/src/calendars/google-calendar/events/format.ts:184">
P2: All-day mutations can be persisted as timed events because `allDay` is ignored; the Google payload must either derive the date/dateTime shape from this flag or reject inputs whose flag and Temporal values disagree, and patches must handle an all-day-only change.</violation>
</file>
<file name="packages/providers/src/calendars/microsoft-calendar/recurrence/format.ts">
<violation number="1" location="packages/providers/src/calendars/microsoft-calendar/recurrence/format.ts:116">
P1: Rules with `BYSETPOS` outside relative monthly/yearly patterns are serialized as if the selector were absent, producing a different recurrence instead of rejecting an unrepresentable rule; validation should allow `BYSETPOS` only when it is consumed by a relative pattern.</violation>
<violation number="2" location="packages/providers/src/calendars/microsoft-calendar/recurrence/format.ts:154">
P1: Monthly and yearly rules that combine `BYDAY` with `BYMONTHDAY` are silently narrowed to the relative weekday pattern because the `byDay` branches ignore `byMonthDay`; rejecting this combination would prevent Microsoft from creating a different series.</violation>
</file>
<file name="packages/providers/src/calendars/google-calendar/conferences.ts">
<violation number="1" location="packages/providers/src/calendars/google-calendar/conferences.ts:271">
P2: In an event update, `formatEventInput` echoes the existing event's conference data through `formatConferenceInput`, and the completed-conference branch blindly dereferences `conferenceData.conferenceSolution!.key!.type!` even though the guard only verified `entryPoints?.length`. If an API response ever returns entry points without `conferenceSolution`, this throws and fails the update for a conference that could otherwise round-trip fine. The `!` here is only a type assertion, not a runtime check. Also, `...conferenceData` re-sends the original `createRequest` (status "success"), which contradicts the branch's stated intent of copying the conference "instead of re-echoing the request". Consider guarding on `conferenceSolution?.key?.type` and building the returned conference object field-by-field (or at least dropping `createRequest`) rather than re-spreading the whole payload.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ...formatRecurrencePatch( | ||
| event.recurrence, | ||
| options.startForRecurrence ?? event.start, | ||
| metadata.recurrenceTimeZone, |
Contributor
There was a problem hiding this comment.
P1: Recurrence-only edits can silently move an existing series into UTC because the formatter receives the patch metadata rather than the master’s stored recurrenceTimeZone; this changes recurrence dates and DST behavior for non-UTC series. Preserve the existing master metadata when resolving a sparse recurrence update and pass that timezone through the formatter.
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 217:
<comment>Recurrence-only edits can silently move an existing series into UTC because the formatter receives the patch metadata rather than the master’s stored `recurrenceTimeZone`; this changes recurrence dates and DST behavior for non-UTC series. Preserve the existing master metadata when resolving a sparse recurrence update and pass that timezone through the formatter.</comment>
<file context>
@@ -0,0 +1,220 @@
+ ...formatRecurrencePatch(
+ event.recurrence,
+ options.startForRecurrence ?? event.start,
+ metadata.recurrenceTimeZone,
+ ),
+ };
</file context>
| headers: { Prefer: TEXT_BODY_PREFERENCE }, | ||
| }); | ||
|
|
||
| await this.eventsFor(sourceCalendar.id).delete({ |
Contributor
There was a problem hiding this comment.
P1: A failed source deletion leaves the original and newly created destination event in place, so this copy-then-delete move can duplicate events and repeat the duplication on retry. The move should validate the source version before copying and provide cleanup/idempotency or an explicit reconciliation path when deletion fails.
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/index.ts, line 516:
<comment>A failed source deletion leaves the original and newly created destination event in place, so this copy-then-delete move can duplicate events and repeat the duplication on retry. The move should validate the source version before copying and provide cleanup/idempotency or an explicit reconciliation path when deletion fails.</comment>
<file context>
@@ -351,9 +491,38 @@ export class MicrosoftCalendarEvents implements CalendarProviderEvents {
+ headers: { Prefer: TEXT_BODY_PREFERENCE },
+ });
+
+ await this.eventsFor(sourceCalendar.id).delete({
+ userId: "me",
+ eventId,
</file context>
| ...shared, | ||
| freq: "MONTHLY", | ||
| ...(pattern.dayOfMonth !== undefined | ||
| ? { byMonthDay: [pattern.dayOfMonth] } |
Contributor
There was a problem hiding this comment.
P1: Graph absoluteMonthly/absoluteYearly rules on dates that do not exist in every target month or year are returned as RFC BYMONTHDAY rules, changing which occurrences users see. The parser should preserve this provider-specific behavior or reject these unsupported recurrences instead of exposing a misleading rule.
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/recurrence/parse.ts, line 66:
<comment>Graph absoluteMonthly/absoluteYearly rules on dates that do not exist in every target month or year are returned as RFC `BYMONTHDAY` rules, changing which occurrences users see. The parser should preserve this provider-specific behavior or reject these unsupported recurrences instead of exposing a misleading rule.</comment>
<file context>
@@ -0,0 +1,96 @@
+ ...shared,
+ freq: "MONTHLY",
+ ...(pattern.dayOfMonth !== undefined
+ ? { byMonthDay: [pattern.dayOfMonth] }
+ : {}),
+ };
</file context>
| return { | ||
| ...parseOriginalStartTimeZone(event), | ||
| ...parseOriginalEndTimeZone(event), | ||
| onlineMeeting: event.onlineMeeting, |
Contributor
There was a problem hiding this comment.
P1: Editing a Microsoft online-meeting event can fail input validation because this stores Graph's raw nullable OnlineMeetingInfo in metadata, but the update schema only permits omitted or non-null values. Normalize nullable fields to undefined/filtered strings before returning metadata so the provider metadata can round-trip through event updates.
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/parse.ts, line 139:
<comment>Editing a Microsoft online-meeting event can fail input validation because this stores Graph's raw nullable `OnlineMeetingInfo` in metadata, but the update schema only permits omitted or non-null values. Normalize nullable fields to `undefined`/filtered strings before returning metadata so the provider metadata can round-trip through event updates.</comment>
<file context>
@@ -0,0 +1,205 @@
+ return {
+ ...parseOriginalStartTimeZone(event),
+ ...parseOriginalEndTimeZone(event),
+ onlineMeeting: event.onlineMeeting,
+ ...parseRecurrenceTimeZone(event),
+ };
</file context>
| } | ||
|
|
||
| function parseDateTime({ dateTime, timeZone }: GoogleCalendarDateTime) { | ||
| const instant = Temporal.Instant.from(dateTime); |
Contributor
There was a problem hiding this comment.
P1: Timed Google events whose dateTime omits an offset and supplies timeZone cannot be parsed: Temporal.Instant.from throws before the supplied timezone is applied. Parse offset-bearing values as instants, but parse local date-times with Temporal.PlainDateTime.from(...).toZonedDateTime(...).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/google-calendar/events/parse.ts, line 57:
<comment>Timed Google events whose `dateTime` omits an offset and supplies `timeZone` cannot be parsed: `Temporal.Instant.from` throws before the supplied timezone is applied. Parse offset-bearing values as instants, but parse local date-times with `Temporal.PlainDateTime.from(...).toZonedDateTime(...)`.</comment>
<file context>
@@ -0,0 +1,261 @@
+}
+
+function parseDateTime({ dateTime, timeZone }: GoogleCalendarDateTime) {
+ const instant = Temporal.Instant.from(dateTime);
+
+ if (!timeZone) {
</file context>
| description: event.description, | ||
| location: event.location, | ||
| visibility: event.visibility, | ||
| start: formatDate(event.start), |
Contributor
There was a problem hiding this comment.
P2: All-day mutations can be persisted as timed events because allDay is ignored; the Google payload must either derive the date/dateTime shape from this flag or reject inputs whose flag and Temporal values disagree, and patches must handle an all-day-only 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/google-calendar/events/format.ts, line 184:
<comment>All-day mutations can be persisted as timed events because `allDay` is ignored; the Google payload must either derive the date/dateTime shape from this flag or reject inputs whose flag and Temporal values disagree, and patches must handle an all-day-only change.</comment>
<file context>
@@ -0,0 +1,297 @@
+ description: event.description,
+ location: event.location,
+ visibility: event.visibility,
+ start: formatDate(event.start),
+ end: formatDate(event.end),
+ transparency: formatTransparency(event),
</file context>
| conferenceSolution: { | ||
| iconUri: conferenceData.conferenceSolution?.iconUri, | ||
| key: { | ||
| type: conferenceData.conferenceSolution!.key!.type!, |
Contributor
There was a problem hiding this comment.
P2: In an event update, formatEventInput echoes the existing event's conference data through formatConferenceInput, and the completed-conference branch blindly dereferences conferenceData.conferenceSolution!.key!.type! even though the guard only verified entryPoints?.length. If an API response ever returns entry points without conferenceSolution, this throws and fails the update for a conference that could otherwise round-trip fine. The ! here is only a type assertion, not a runtime check. Also, ...conferenceData re-sends the original createRequest (status "success"), which contradicts the branch's stated intent of copying the conference "instead of re-echoing the request". Consider guarding on conferenceSolution?.key?.type and building the returned conference object field-by-field (or at least dropping createRequest) rather than re-spreading the whole payload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/providers/src/calendars/google-calendar/conferences.ts, line 271:
<comment>In an event update, `formatEventInput` echoes the existing event's conference data through `formatConferenceInput`, and the completed-conference branch blindly dereferences `conferenceData.conferenceSolution!.key!.type!` even though the guard only verified `entryPoints?.length`. If an API response ever returns entry points without `conferenceSolution`, this throws and fails the update for a conference that could otherwise round-trip fine. The `!` here is only a type assertion, not a runtime check. Also, `...conferenceData` re-sends the original `createRequest` (status "success"), which contradicts the branch's stated intent of copying the conference "instead of re-echoing the request". Consider guarding on `conferenceSolution?.key?.type` and building the returned conference object field-by-field (or at least dropping `createRequest`) rather than re-spreading the whole payload.</comment>
<file context>
@@ -228,3 +230,70 @@ export function toConferenceData(
+ conferenceSolution: {
+ iconUri: conferenceData.conferenceSolution?.iconUri,
+ key: {
+ type: conferenceData.conferenceSolution!.key!.type!,
+ },
+ name: conferenceData.conferenceSolution?.name,
</file context>
| queryCache: new QueryCache({ | ||
| onError: (error) => { | ||
| console.error(error.message ?? "Something went wrong"); | ||
| console.error(error.message); |
Contributor
There was a problem hiding this comment.
P3: Removing the ?? "Something went wrong" fallback means that when an error has no message (e.g. a thrown non-Error value), the console now logs undefined instead of the earlier friendly message. Consider keeping a fallback for robustness.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/lib/trpc/query-client.ts, line 32:
<comment>Removing the `?? "Something went wrong"` fallback means that when an error has no message (e.g. a thrown non-Error value), the console now logs `undefined` instead of the earlier friendly message. Consider keeping a fallback for robustness.</comment>
<file context>
@@ -29,7 +29,7 @@ export function makeQueryClient() {
queryCache: new QueryCache({
onError: (error) => {
- console.error(error.message ?? "Something went wrong");
+ console.error(error.message);
},
}),
</file context>
| import type { CalendarEvent, Meeting } from "../interfaces/events"; | ||
|
|
||
| export function isMeeting(event: CalendarEvent): event is Meeting { | ||
| return (event.attendees?.length ?? 0) > 1; |
Contributor
There was a problem hiding this comment.
P3: Heuristic note: classifying "is a meeting" purely by attendees?.length > 1 ignores attendee roles/status. An event whose only other attendee is a resource room, or a meeting whose counterpart declined (or only the organizer is listed), will be misclassified either way. Since Attendee carries type (required/optional/resource), organizer, and status, counting only non-resource, required attendees (or checking for a conference) would match the intent more reliably.
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>Heuristic note: classifying "is a meeting" purely by `attendees?.length > 1` ignores attendee roles/status. An event whose only other attendee is a resource room, or a meeting whose counterpart declined (or only the organizer is listed), will be misclassified either way. Since Attendee carries `type` (required/optional/resource), `organizer`, and `status`, counting only non-resource, required attendees (or checking for a conference) would match the intent more reliably.</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?.length ?? 0) > 1;
+}
</file context>
| async move() { | ||
| return this.withErrorHandler("events.move", () => { | ||
| throw new Error("Moving Microsoft Calendar events is not supported"); | ||
| async move({ |
Contributor
There was a problem hiding this comment.
P3: The added Microsoft move implementation is dead from the application's current entry points: the API explicitly rejects Microsoft moves and its standalone move schema only accepts Google calendars. Either wire Microsoft into the move routing/validation or leave this method unsupported until a caller exists.
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/index.ts, line 494:
<comment>The added Microsoft `move` implementation is dead from the application's current entry points: the API explicitly rejects Microsoft moves and its standalone move schema only accepts Google calendars. Either wire Microsoft into the move routing/validation or leave this method unsupported until a caller exists.</comment>
<file context>
@@ -351,9 +491,38 @@ export class MicrosoftCalendarEvents implements CalendarProviderEvents {
- async move() {
- return this.withErrorHandler("events.move", () => {
- throw new Error("Moving Microsoft Calendar events is not supported");
+ async move({
+ sourceCalendar,
+ destinationCalendar,
</file context>
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.
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
Reorganized calendar providers into clear parse/format modules, added solid recurrence/conference handling, and tightened types across Google and Microsoft. This reduces bugs, supports delta “removed” events, and makes provider code easier to maintain.
Refactors
@repo/providersGoogle/Microsoft code intoparseandformatmodules for calendars, events, freebusy; moved conference helpers toconferences.DeltaRemovedEventin all delta APIs.Partial<Event>;CalendarProviderEventsListOptions.timeZoneis now required;sendUpdatedefaults to true in events create.Migration
parseConferenceData->parseConference,toGoogleCalendarEventInput->formatEventInput, Microsoft event helpers toevents/formatandevents/parse.timeZonetoCalendarProvider.events.list.Partial<Event>; handle delta results that may include removed items.@repo/providers/libisMeetingif needed; old meetings util was removed.Written for commit 87d83f3. Summary will update on new commits.