Conversation
…dling
Fix calendar OAuth callbacks to redirect to /{emailAccountId}/calendars
and standardize error handling between Google and Microsoft providers.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds Microsoft (Outlook) calendar support alongside Google: new OAuth routes, provider implementations, unified availability aggregation, shared callback/helpers, UI updates for multi-provider connection, and migration of redirect base URLs to env.NEXT_PUBLIC_BASE_URL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client UI
participant API as /api/outlook/calendar/auth-url
participant OAuth as Microsoft OAuth
participant Callback as /api/outlook/calendar/callback
participant Handler as handleCalendarCallback
participant Provider as microsoftCalendarProvider
participant DB as Database
User->>UI: Click "Add Outlook Calendar"
UI->>API: GET /api/outlook/calendar/auth-url (scoped)
API->>API: create state cookie (emailAccountId, nonce)
API-->>UI: { url }
UI->>OAuth: Redirect to provider URL
OAuth->>Callback: Redirect with code & state
Callback->>Handler: Delegate(request, microsoftCalendarProvider, logger)
Handler->>Handler: validate callback & parse state
Handler->>DB: verify email account ownership
Handler->>Provider: exchangeCodeForTokens(code)
Provider-->>Handler: tokens + email
Handler->>DB: check/create calendarConnection
Handler->>Provider: syncCalendars(connectionId, tokens...)
Provider->>DB: upsert calendars
Handler-->>UI: Redirect to /calendars?connected=1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (13)
apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnectionCard.tsx (1)
31-46: Consider logging unrecognized providers.The fallback to Google for unrecognized providers provides graceful degradation, but silently defaulting could mask configuration issues or bugs if an unexpected provider value appears in the database.
Consider adding a console warning:
const getProviderInfo = (provider: string) => { const providers = { microsoft: { name: "Microsoft Calendar", icon: "/images/product/outlook-calendar.svg", alt: "Microsoft Calendar", }, google: { name: "Google Calendar", icon: "/images/product/google-calendar.svg", alt: "Google Calendar", }, }; + if (!(provider in providers)) { + console.warn(`Unknown calendar provider: ${provider}, falling back to Google`); + } + return providers[provider as keyof typeof providers] || providers.google; };apps/web/utils/calendar/availability-types.ts (1)
12-20: Consider simplifying theaccessTokentype.The
accessTokenparameter is both optional (?) and nullable (| null), which is redundant. Consider using justaccessToken: string | nullto simplify the type signature, unless there's a semantic distinction betweenundefinedandnullin your usage.fetchBusyPeriods(params: { - accessToken?: string | null; + accessToken: string | null; refreshToken: string | null; expiresAt: number | null; emailAccountId: string; calendarIds: string[]; timeMin: string; timeMax: string; }): Promise<BusyPeriod[]>;apps/web/utils/ai/calendar/availability.ts (1)
112-142: Validate tool date inputs before calling unified availability
executerelies onnew Date(timeMin/timeMax)from unconstrainedstringinputs, which can yieldInvalid Dateor timezone‑dependent results if the model sends non‑ISO or ambiguous values. Consider tightening theinputSchema(e.g., ISO 8601 withz.string().datetime()) or explicitly validating and short‑circuiting on invalid dates before callinggetUnifiedCalendarAvailability, returning an empty result or an error shape instead.apps/web/utils/calendar/unified-availability.ts (2)
25-31: Confirm widening the requested window to full days is intentional
startDate/endDateare expanded tostartOfDay/endOfDayin the user’s timezone before querying providers. This effectively turns any narrower caller window into whole‑day (or multi‑day) availability checks. If upstream callers intend to respect more precise time ranges, consider either:
- Honoring the exact
startDate/endDatewithout day expansion, or- Renaming the parameters / documenting that this API always works on full days so callers pass only dates.
124-136: Optionally sort (and later dedupe) unified busy periods for downstream consumersThe merged
allBusyPeriods = results.flat()is returned in provider/connection iteration order. For easier downstream handling (especially UI or LLM tools), consider sorting bystart(and, if needed, merging or deduping overlapping periods) before returning.apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (1)
7-8: CentralizeGetCalendarAuthUrlResponseinstead of importing from a specific routeThis client component uses
GetCalendarAuthUrlResponsefor both Google and Outlook but imports the type from the Google route module. To avoid coupling client code to a specific route file (and to keep Google/Outlook in sync), consider moving this shared response type to a common calendar types module and importing it from there in both routes andConnectCalendar.apps/web/utils/calendar/handle-calendar-callback.ts (1)
117-136: Preserve granular error codes fromRedirectErrorinstead of always forcingconnection_failed.
RedirectErrorinstances are thrown from helpers that already set specificerrorquery params onredirectUrl(missing_code,invalid_state,unauthorized, etc.), but the catch block always overwrites them with"connection_failed". This can reduce debuggability and surface less useful messages to the UI.Consider reusing any existing
errorquery param when handlingRedirectError, falling back to"connection_failed"only if none is present:- if (error instanceof RedirectError) { - return redirectWithError( - error.redirectUrl, - "connection_failed", - error.responseHeaders, - ); - } + if (error instanceof RedirectError) { + const existingError = + error.redirectUrl.searchParams.get("error") ?? "connection_failed"; + return redirectWithError( + error.redirectUrl, + existingError, + error.responseHeaders, + ); + }This keeps the shared handler generic while preserving the more specific semantics encoded by the lower-level helpers.
apps/web/utils/calendar/providers/microsoft-availability.ts (1)
11-61: Verify thatcalendarIdsmap correctly to Microsoft Graphschedulesidentifiers.
fetchMicrosoftCalendarBusyPeriodscalls:await calendarClient.api("/me/calendar/getSchedule").post({ schedules: calendarIds, ... });The Graph
getScheduleAPI typically expects mailbox identifiers (often email addresses or user IDs) inschedules. Upstream, these are namedcalendarIds, which might instead be calendar resource IDs.Please double‑check that the values passed in
calendarIdsare validschedulesidentifiers for Graph (e.g., mailbox addresses). If they are mailbox addresses, consider renaming to make that intent clearer; if they are calendar IDs, you may need to adapt the call or mapping.apps/web/utils/calendar/providers/microsoft.ts (1)
20-77: Add a defensive check fortokens.access_tokenbefore using it.After exchanging the authorization code, the code assumes
tokens.access_tokenis present and immediately uses it to callhttps://graph.microsoft.com/v1.0/me. While this is expected in normal responses, a malformed or partial token response would result in an Authorization header withBearer undefined, leading to a harder‑to‑diagnose failure.A small guard would make this more robust and yield a clearer error:
- const tokens = await tokenResponse.json(); - - if (!tokenResponse.ok) { - throw new Error( - tokens.error_description || "Failed to exchange code for tokens", - ); - } + const tokens = await tokenResponse.json(); + + if (!tokenResponse.ok) { + throw new Error( + tokens.error_description || "Failed to exchange code for tokens", + ); + } + + if (!tokens.access_token) { + throw new Error("Token response missing access_token"); + }This keeps failures in the token step clearly separated from Graph profile issues.
apps/web/utils/outlook/calendar-client.ts (1)
96-113: Clarify or tighten the lookup used when saving refreshed tokens.In
getCalendarClientWithRefresh, the refreshed tokens are persisted to the firstcalendarConnectionfound by:const calendarConnection = await prisma.calendarConnection.findFirst({ where: { emailAccountId, provider: "microsoft", }, select: { id: true }, });This assumes there is at most one Microsoft calendar connection per
emailAccountId. If that assumption ever changes (e.g., multiple Microsoft connections per email account), this would update an arbitrary one.Either:
- Document this 1:1 assumption in a short comment, or
- Tighten the lookup (e.g., use a unique constraint and
findUnique, or include additional fields such asThis will make the refresh behavior more future‑proof and easier to reason about.
apps/web/utils/calendar/oauth-callback-helpers.ts (3)
16-20: Consider extracting validation bounds as named constants.The validation bounds (max 64 for
emailAccountId, min 8/max 128 fornonce) are magic numbers. Consider extracting these as named constants to improve maintainability and ensure consistency with database constraints.const EMAIL_ACCOUNT_ID_MAX_LENGTH = 64; const NONCE_MIN_LENGTH = 8; const NONCE_MAX_LENGTH = 128; const calendarOAuthStateSchema = z.object({ emailAccountId: z.string().min(1).max(EMAIL_ACCOUNT_ID_MAX_LENGTH), type: z.literal("calendar"), nonce: z.string().min(NONCE_MIN_LENGTH).max(NONCE_MAX_LENGTH), });
69-94: Simplify the type assertion in parseOAuthState call.The type assertion
parseOAuthState<Omit<CalendarOAuthState, "nonce">>(storedState)is unnecessarily complex. SinceparseOAuthStatereturnsT & { nonce: string }, the result is alreadyCalendarOAuthState. You can simplify this to improve readability.let rawState: unknown; try { - rawState = parseOAuthState<Omit<CalendarOAuthState, "nonce">>(storedState); + rawState = parseOAuthState(storedState); } catch (error) { logger.error("Failed to decode state", { error }); redirectUrl.searchParams.set("error", "invalid_state_format"); throw new RedirectError(redirectUrl, responseHeaders); }
205-218: Consider adding JSDoc to document the RedirectError pattern.The custom
RedirectErrorclass is a useful pattern for propagating redirect responses through error handling. Adding JSDoc would help other developers understand when and how to use this pattern./** * Custom error class for redirect responses + * + * Thrown when an OAuth callback validation error occurs and the user should + * be redirected back to the calendar page with appropriate error information. + * The error carries the redirect URL and response headers needed to construct + * the redirect response. + * + * @example + * ```typescript + * throw new RedirectError(redirectUrl, response.headers); + * ``` */ export class RedirectError extends Error {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/web/public/images/product/outlook-calendar.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
README.md(1 hunks)apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnectionCard.tsx(3 hunks)apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnections.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendarButton.tsx(0 hunks)apps/web/app/(app)/[emailAccountId]/calendars/page.tsx(1 hunks)apps/web/app/api/google/calendar/callback/route.ts(1 hunks)apps/web/app/api/google/linking/callback/route.ts(5 hunks)apps/web/app/api/mcp/[integration]/callback/route.ts(2 hunks)apps/web/app/api/outlook/calendar/auth-url/route.ts(1 hunks)apps/web/app/api/outlook/calendar/callback/route.ts(1 hunks)apps/web/app/api/outlook/linking/callback/route.ts(5 hunks)apps/web/utils/ai/calendar/availability.ts(3 hunks)apps/web/utils/calendar/availability-types.ts(1 hunks)apps/web/utils/calendar/availability.ts(0 hunks)apps/web/utils/calendar/handle-calendar-callback.ts(1 hunks)apps/web/utils/calendar/oauth-callback-helpers.ts(1 hunks)apps/web/utils/calendar/oauth-types.ts(1 hunks)apps/web/utils/calendar/providers/google-availability.ts(1 hunks)apps/web/utils/calendar/providers/google.ts(1 hunks)apps/web/utils/calendar/providers/microsoft-availability.ts(1 hunks)apps/web/utils/calendar/providers/microsoft.ts(1 hunks)apps/web/utils/calendar/unified-availability.ts(1 hunks)apps/web/utils/oauth/account-linking.ts(2 hunks)apps/web/utils/oauth/callback-validation.ts(2 hunks)apps/web/utils/outlook/calendar-client.ts(1 hunks)apps/web/utils/outlook/scopes.ts(1 hunks)package.json(1 hunks)version.txt(1 hunks)
💤 Files with no reviewable changes (2)
- apps/web/utils/calendar/availability.ts
- apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendarButton.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-08T13:14:07.449Z
Learnt from: elie222
Repo: elie222/inbox-zero PR: 537
File: apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx:30-34
Timestamp: 2025-07-08T13:14:07.449Z
Learning: The clean onboarding page in apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx is intentionally Gmail-specific and should show an error for non-Google email accounts rather than attempting to support multiple providers.
Applied to files:
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsxapps/web/app/(app)/[emailAccountId]/calendars/CalendarConnections.tsx
🧬 Code graph analysis (19)
apps/web/utils/calendar/unified-availability.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/availability-types.ts (1)
BusyPeriod(1-4)apps/web/utils/calendar/providers/google-availability.ts (1)
googleAvailabilityProvider(63-89)apps/web/utils/calendar/providers/microsoft-availability.ts (1)
microsoftAvailabilityProvider(68-94)
apps/web/utils/oauth/account-linking.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/utils/calendar/providers/google-availability.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/availability-types.ts (2)
BusyPeriod(1-4)CalendarAvailabilityProvider(6-21)
apps/web/utils/calendar/providers/google.ts (3)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/oauth-types.ts (2)
CalendarOAuthProvider(10-28)CalendarTokens(3-8)apps/web/utils/calendar/client.ts (2)
getCalendarOAuth2Client(31-37)fetchGoogleCalendars(117-127)
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (4)
apps/web/providers/EmailAccountProvider.tsx (1)
useAccount(79-89)apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/fetch.ts (1)
fetchWithAccount(7-25)apps/web/app/api/outlook/calendar/auth-url/route.ts (1)
GetCalendarAuthUrlResponse(10-10)
apps/web/app/api/outlook/calendar/callback/route.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/middleware.ts (1)
withError(327-345)apps/web/utils/calendar/handle-calendar-callback.ts (1)
handleCalendarCallback(20-138)apps/web/utils/calendar/providers/microsoft.ts (1)
microsoftCalendarProvider(12-131)
apps/web/utils/oauth/callback-validation.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/app/api/outlook/calendar/auth-url/route.ts (5)
apps/web/utils/oauth/state.ts (2)
generateOAuthState(10-18)oauthStateCookieOptions(34-40)apps/web/utils/outlook/calendar-client.ts (1)
getCalendarOAuth2Url(25-42)apps/web/app/api/outlook/calendar/callback/route.ts (1)
GET(8-10)apps/web/utils/middleware.ts (1)
withEmailAccount(373-391)apps/web/utils/calendar/constants.ts (1)
CALENDAR_STATE_COOKIE_NAME(1-1)
apps/web/app/api/google/linking/callback/route.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/utils/calendar/providers/microsoft-availability.ts (2)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/availability-types.ts (2)
BusyPeriod(1-4)CalendarAvailabilityProvider(6-21)
apps/web/app/api/mcp/[integration]/callback/route.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/utils/calendar/providers/microsoft.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/oauth-types.ts (2)
CalendarOAuthProvider(10-28)CalendarTokens(3-8)apps/web/env.ts (1)
env(16-244)apps/web/utils/outlook/calendar-client.ts (2)
getCalendarClientWithRefresh(44-135)fetchMicrosoftCalendars(137-153)
apps/web/utils/calendar/handle-calendar-callback.ts (4)
apps/web/utils/calendar/oauth-types.ts (1)
CalendarOAuthProvider(10-28)apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/calendar/oauth-callback-helpers.ts (9)
validateOAuthCallback(25-64)parseAndValidateCalendarState(69-94)buildCalendarRedirectUrl(99-104)verifyEmailAccountAccess(109-138)checkExistingConnection(143-155)redirectWithMessage(184-191)createCalendarConnection(160-179)RedirectError(208-218)redirectWithError(196-203)apps/web/env.ts (1)
env(16-244)
apps/web/utils/outlook/calendar-client.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/env.ts (1)
env(16-244)apps/web/utils/outlook/scopes.ts (1)
CALENDAR_SCOPES(20-28)apps/web/utils/error.ts (1)
SafeError(59-69)
apps/web/app/(app)/[emailAccountId]/calendars/page.tsx (3)
apps/web/components/PageWrapper.tsx (1)
PageWrapper(3-20)apps/web/components/PageHeader.tsx (1)
PageHeader(14-36)apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (1)
ConnectCalendar(12-111)
apps/web/utils/ai/calendar/availability.ts (1)
apps/web/utils/calendar/unified-availability.ts (1)
getUnifiedCalendarAvailability(14-137)
apps/web/utils/calendar/oauth-callback-helpers.ts (6)
apps/web/utils/logger.ts (1)
Logger(5-5)apps/web/utils/calendar/oauth-types.ts (2)
OAuthCallbackValidation(30-34)CalendarOAuthState(36-40)apps/web/utils/calendar/constants.ts (1)
CALENDAR_STATE_COOKIE_NAME(1-1)apps/web/env.ts (1)
env(16-244)apps/web/utils/oauth/state.ts (1)
parseOAuthState(25-29)apps/web/utils/auth.ts (1)
auth(495-496)
apps/web/app/api/google/calendar/callback/route.ts (2)
apps/web/utils/calendar/handle-calendar-callback.ts (1)
handleCalendarCallback(20-138)apps/web/utils/calendar/providers/google.ts (1)
googleCalendarProvider(13-99)
apps/web/app/api/outlook/linking/callback/route.ts (1)
apps/web/env.ts (1)
env(16-244)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Software Component Analysis Js
- GitHub Check: Static Code Analysis Js
- GitHub Check: Software Component Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
- GitHub Check: Static Code Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
🔇 Additional comments (28)
version.txt (1)
1-1: LGTM! Appropriate version bump for feature release.The minor version increment to v2.20.0 correctly signals the addition of Microsoft Calendar support as a new feature.
apps/web/utils/oauth/callback-validation.ts (1)
1-32: LGTM! Consistent refactoring to use environment-based base URL.The removal of the
baseUrlparameter and the use ofenv.NEXT_PUBLIC_BASE_URLsimplifies the function signature and centralizes redirect URL configuration. This change is consistent with the broader refactoring across OAuth and callback handling in this PR.README.md (1)
414-431: LGTM! Clear and comprehensive Microsoft Calendar setup documentation.The documentation provides all necessary steps for configuring Microsoft Calendar integration, including Azure app registration, redirect URI setup, and required permissions. The note about the calendar OAuth flow being separate from the email OAuth is particularly helpful.
apps/web/utils/oauth/account-linking.ts (1)
1-30: LGTM! Consistent refactoring pattern applied.The removal of the
baseUrlparameter and the use ofenv.NEXT_PUBLIC_BASE_URLaligns with the environment-based redirect URL pattern introduced across the PR. This simplifies the function signature and centralizes configuration.apps/web/app/api/google/linking/callback/route.ts (1)
46-46: LGTM! Consistent application of environment-based redirect URLs.All redirect URL constructions in this file have been updated to use
env.NEXT_PUBLIC_BASE_URLinstead ofrequest.nextUrl.origin. This ensures consistent redirect behavior across all code paths in the Google linking callback.Also applies to: 60-60, 203-203, 241-241, 250-250
apps/web/utils/outlook/scopes.ts (1)
20-28: LGTM! Appropriate Microsoft Graph calendar scopes defined.The
CALENDAR_SCOPESconstant includes the necessary permissions for calendar integration:
- Baseline authentication scopes (openid, profile, email, User.Read)
offline_accessfor refresh tokensCalendars.ReadandCalendars.ReadWritefor calendar accessThis follows the established pattern of the existing
SCOPESconstant and aligns with Microsoft Graph API requirements.apps/web/app/api/mcp/[integration]/callback/route.ts (1)
53-53: Environment variable properly configured and verified.The refactoring is sound:
NEXT_PUBLIC_BASE_URLis consistently applied across all OAuth and callback routes. Verification confirms:
- Required field: Defined as
z.string()inenv.ts(no optional flag), enforcing configuration across all environments- Documented:
.env.exampleprovides the development default; README explicitly documents values for dev, Docker, and production deployments- Build-time requirement: Docker deployment instructions clearly state it must be passed as
--build-arg(correctly handled by Next.js build process)- No redirect URL vulnerabilities: All redirect URLs construct from the required environment variable; validation checks remain intact
No actionable items remain.
package.json (1)
21-21: Turbo 2.6.1 and pnpm 10.22.0 are compatible—no breaking changes detected.Turbo 2.6.1 contains bug fixes and documentation updates with no advertised breaking changes compared to 2.5.8, and pnpm 10.22.0 has no breaking changes listed. Both updates introduce new features without compromising existing functionality. The versions are safe to use.
apps/web/utils/calendar/oauth-types.ts (1)
1-40: LGTM!The type definitions are well-structured and provide a clean abstraction for calendar OAuth providers. The interfaces clearly separate concerns between token exchange, calendar synchronization, and OAuth state management.
apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnections.tsx (1)
17-17: LGTM!The updated message accurately reflects the new multi-provider calendar support introduced in this PR.
apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnectionCard.tsx (1)
57-57: LGTM!The dynamic provider rendering correctly uses the computed provider information to display the appropriate icon, alt text, and title for each calendar connection.
Also applies to: 125-132
apps/web/app/(app)/[emailAccountId]/calendars/page.tsx (1)
4-4: LGTM!The component replacement and responsive layout adjustments properly support the new multi-provider calendar connection flow. The layout will stack vertically on smaller screens and arrange horizontally on larger screens, providing a better user experience.
Also applies to: 9-15
apps/web/app/api/outlook/calendar/callback/route.ts (1)
1-10: LGTM!The route handler follows a clean, consistent pattern by delegating to the unified
handleCalendarCallbackhandler with the Microsoft provider. The implementation is well-scoped with proper logging and error handling.apps/web/app/api/outlook/calendar/auth-url/route.ts (2)
12-21: LGTM!The
getAuthUrlhelper properly generates OAuth state with the email account context and delegates URL construction to the shared Outlook calendar client utility.
23-37: LGTM!The route handler correctly:
- Uses
withEmailAccountmiddleware to ensure proper authentication- Generates the OAuth URL with appropriate state
- Sets the state cookie with secure options
- Returns a properly typed response
apps/web/app/api/outlook/linking/callback/route.ts (1)
49-49: All environment variable configurations verified and properly implemented.The changes have been verified across all mentioned lines (49, 63, 274, 310, 319). The environment variable is properly configured in
env.tswith required string validation (z.string()), correctly imported in the route handler, and consistently used for all redirect URL constructions. No remaining uses ofrequest.nextUrl.originexist in the OAuth callbacks.apps/web/utils/calendar/providers/google-availability.ts (1)
11-88: Google FreeBusy wrapper and provider wiring look correctThe FreeBusy query, BusyPeriod extraction, and integration with
getCalendarClientWithRefreshalign with the provider contract and unified availability usage. Error handling and trace logging are appropriate.apps/web/utils/calendar/providers/google.ts (1)
16-97: Google OAuth token exchange and calendar sync flow look solidThe provider correctly validates the ID token/email, enforces presence of a refresh token, and syncs calendars via upsert while marking the connection disconnected on failure. The integration with
getCalendarClientWithRefreshand Prisma schema appears coherent.apps/web/app/api/google/calendar/callback/route.ts (1)
3-9: Thin delegation to shared calendar callback handler looks good.Routing Google calendar callbacks through
handleCalendarCallbackviagoogleCalendarProviderkeeps this route minimal and centralizes the OAuth and syncing flow in one place. The logger scope remains specific to this route, which is helpful for tracing.apps/web/utils/calendar/oauth-callback-helpers.ts (9)
1-14: LGTM!Imports are well-organized and all appear to be used appropriately throughout the file.
34-41: LGTM! Strong origin validation.Good security practice to validate the OAuth callback origin against the expected base URL. This prevents redirect attacks and ensures callbacks come from the expected domain.
54-61: LGTM! Proper CSRF protection via state validation.Excellent implementation of OAuth state validation to prevent CSRF attacks. The comparison ensures the state parameter matches the stored cookie value.
96-104: LGTM!Clean implementation that properly constructs account-specific calendar URLs using the utility function and environment configuration.
106-138: LGTM! Robust authorization checks.Excellent implementation of authorization verification:
- Validates active session exists
- Confirms user owns the email account via database lookup
- Uses minimal select for efficiency
- Provides appropriate error handling and logging
140-155: LGTM!Straightforward implementation that correctly checks for existing calendar connections using all relevant identifying fields.
181-191: LGTM!Clean helper function for success redirects with proper query parameter handling.
193-203: LGTM!Clean helper function for error redirects with proper query parameter handling.
157-179: The concern about plaintext token storage is incorrect.The tokens are automatically encrypted before storage via Prisma extensions. The
calendarConnectionmodel is explicitly covered by theencryptedTokensextension (line 21 ofapps/web/utils/prisma-extensions.ts), which:
- Encrypts
accessTokenandrefreshTokenon all create/update operations using AES-256-GCM with random IVs and authentication tags- Automatically decrypts them when retrieved
The code already implements secure token storage by design.
Likely an incorrect or invalid review comment.
| const redirectUrl = new URL("/calendars", env.NEXT_PUBLIC_BASE_URL); | ||
| const response = NextResponse.redirect(redirectUrl); |
There was a problem hiding this comment.
Inconsistent redirect URL construction.
Line 43 hardcodes the redirect to /calendars, while buildCalendarRedirectUrl (lines 99-104) uses prefixPath(emailAccountId, "/calendars") to construct a more specific path. This inconsistency could lead to users being redirected to the wrong location.
Consider using buildCalendarRedirectUrl in validateOAuthCallback once the emailAccountId is parsed from state, or document why the generic /calendars redirect is used initially (perhaps as a fallback before state parsing).
Also applies to: 99-104
🤖 Prompt for AI Agents
In apps/web/utils/calendar/oauth-callback-helpers.ts around lines 43-44 (and
also affecting 99-104), the redirect is hardcoded to "/calendars" which is
inconsistent with buildCalendarRedirectUrl that prefixes the path with the
emailAccountId; parse the emailAccountId from the OAuth state before issuing the
redirect and call buildCalendarRedirectUrl(emailAccountId, "/calendars") to
construct the target URL, and if the state is missing or parsing fails keep a
documented fallback to new URL("/calendars", env.NEXT_PUBLIC_BASE_URL) so users
aren’t left without a redirect.
| if (!code || code.length < 10) { | ||
| logger.warn("Missing or invalid code in calendar callback"); | ||
| redirectUrl.searchParams.set("error", "missing_code"); | ||
| throw new RedirectError(redirectUrl, response.headers); | ||
| } |
There was a problem hiding this comment.
Replace magic number with a more lenient or provider-aware validation.
The hardcoded 10 character minimum for the authorization code is arbitrary. OAuth authorization codes vary significantly in length across providers (Google, Microsoft, etc.). This could reject valid codes or fail to catch malformed ones.
Consider either:
- Removing the length check entirely (presence check is sufficient)
- Using a more lenient minimum (e.g., 5)
- Making it provider-specific if needed
- if (!code || code.length < 10) {
+ if (!code || code.trim().length === 0) {
logger.warn("Missing or invalid code in calendar callback");
redirectUrl.searchParams.set("error", "missing_code");
throw new RedirectError(redirectUrl, response.headers);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!code || code.length < 10) { | |
| logger.warn("Missing or invalid code in calendar callback"); | |
| redirectUrl.searchParams.set("error", "missing_code"); | |
| throw new RedirectError(redirectUrl, response.headers); | |
| } | |
| if (!code || code.trim().length === 0) { | |
| logger.warn("Missing or invalid code in calendar callback"); | |
| redirectUrl.searchParams.set("error", "missing_code"); | |
| throw new RedirectError(redirectUrl, response.headers); | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/calendar/oauth-callback-helpers.ts around lines 48 to 52, the
authorization-code validation uses a hardcoded minimum length of 10 which is
arbitrary and may reject valid provider codes; remove the strict length check
and only validate presence (i.e., ensure code is truthy), or if you prefer a
lenient guard use a much smaller minimum like 5 or implement provider-specific
checks based on the current provider; update the logger/redirect to only treat
missing/empty codes as errors and adjust tests accordingly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
apps/web/utils/outlook/calendar-client.ts (2)
110-110: Improve clarity of expiry calculation.While JavaScript operator precedence makes this work correctly, applying
Math.floorto the final sum can cause precision issues. It's clearer and more precise to floor the milliseconds-to-seconds conversion first.Apply this diff for better precision and clarity:
- expires_at: Math.floor(Date.now() / 1000 + Number(tokens.expires_in)), + expires_at: Math.floor(Date.now() / 1000) + Number(tokens.expires_in),
86-90: Avoid exposing raw error messages from external services.The error description from Microsoft's token endpoint might contain sensitive information or internal details that shouldn't be directly exposed.
Apply this diff to use a generic error message:
if (!response.ok) { - throw new Error(tokens.error_description || "Failed to refresh token"); + logger.error("Token refresh failed", { error: tokens.error_description }); + throw new Error("Failed to refresh token"); }apps/web/__tests__/e2e/calendar/google-calendar.test.ts (1)
149-178: Avoid hard‑coding “at least one busy period” to reduce flakinessThe assertion that there must be at least one busy period tomorrow makes this test depend on personal calendar contents. If the test account has no events tomorrow, you’ll get a failing test even though the integration is working.
Consider treating “no busy periods” as non‑fatal, and still validating shape when data exists. For example:
- // Verify at least one busy period was found - expect(busyPeriods.length).toBeGreaterThan(0); - - // Verify busy periods have correct structure - if (busyPeriods.length > 0) { - expect(busyPeriods[0]).toHaveProperty("start"); - expect(busyPeriods[0]).toHaveProperty("end"); - expect(typeof busyPeriods[0].start).toBe("string"); - expect(typeof busyPeriods[0].end).toBe("string"); - } + // If there are no busy periods, we already logged a warning above; treat as non‑fatal + if (busyPeriods.length === 0) { + return; + } + + // Verify at least one busy period was found + expect(busyPeriods.length).toBeGreaterThan(0); + + // Verify busy periods have correct structure + expect(busyPeriods[0]).toHaveProperty("start"); + expect(busyPeriods[0]).toHaveProperty("end"); + expect(typeof busyPeriods[0].start).toBe("string"); + expect(typeof busyPeriods[0].end).toBe("string");This keeps the assertions meaningful while avoiding spurious failures when the calendar is legitimately empty.
apps/web/__tests__/e2e/calendar/microsoft-calendar.test.ts (1)
153-183: Relax “at least one busy period” assumption to make the test less brittleLike the Google test, this assumes there is at least one event tomorrow:
// (Assuming your calendar actually has events on tomorrow) expect(busyPeriods.length).toBeGreaterThan(0);If the test account is legitimately free tomorrow, the integration is still healthy but the test fails.
You can use the same pattern as the Google test suggestion and early‑return when there are no busy periods, while still asserting structure when data exists:
- // Verify at least one busy period was found - // (Assuming your calendar actually has events on tomorrow) - expect(busyPeriods.length).toBeGreaterThan(0); - - // Verify busy periods have correct structure - if (busyPeriods.length > 0) { - expect(busyPeriods[0]).toHaveProperty("start"); - expect(busyPeriods[0]).toHaveProperty("end"); - expect(typeof busyPeriods[0].start).toBe("string"); - expect(typeof busyPeriods[0].end).toBe("string"); - } + // If there are no busy periods, we already logged a warning above; treat as non‑fatal + if (busyPeriods.length === 0) { + return; + } + + // Verify at least one busy period was found + expect(busyPeriods.length).toBeGreaterThan(0); + + // Verify busy periods have correct structure + expect(busyPeriods[0]).toHaveProperty("start"); + expect(busyPeriods[0]).toHaveProperty("end"); + expect(typeof busyPeriods[0].start).toBe("string"); + expect(typeof busyPeriods[0].end).toBe("string");That keeps the e2e useful without tying it to a specific calendar state.
apps/web/utils/calendar/unified-availability.ts (1)
72-108: Consider logging when Google connections have no enabled calendars (for parity with Microsoft)For Google connections you silently
continuewhen there are no enabled calendars, while for Microsoft you log a warning. For troubleshooting, it can be helpful to have consistent observability across providers.You could mirror the Microsoft behavior like this:
- // Fetch Google calendar availability - for (const connection of googleConnections) { - const calendarIds = connection.calendars.map((cal) => cal.calendarId); - if (!calendarIds.length) continue; + // Fetch Google calendar availability + for (const connection of googleConnections) { + const calendarIds = connection.calendars.map((cal) => cal.calendarId); + if (!calendarIds.length) { + logger.warn("No enabled calendars for Google connection", { + connectionId: connection.id, + }); + continue; + }Not required for correctness, but it will make debugging “no data” scenarios much easier.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/__tests__/e2e/calendar/google-calendar.test.ts(1 hunks)apps/web/__tests__/e2e/calendar/microsoft-calendar.test.ts(1 hunks)apps/web/utils/calendar/providers/microsoft-availability.ts(1 hunks)apps/web/utils/calendar/unified-availability.ts(1 hunks)apps/web/utils/outlook/calendar-client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/calendar/providers/microsoft-availability.ts
🧰 Additional context used
🧬 Code graph analysis (4)
apps/web/utils/outlook/calendar-client.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/env.ts (1)
env(16-244)apps/web/utils/outlook/scopes.ts (1)
CALENDAR_SCOPES(20-28)apps/web/utils/error.ts (1)
SafeError(59-69)
apps/web/__tests__/e2e/calendar/google-calendar.test.ts (1)
apps/web/utils/calendar/providers/google-availability.ts (1)
googleAvailabilityProvider(63-89)
apps/web/utils/calendar/unified-availability.ts (4)
apps/web/utils/logger.ts (1)
createScopedLogger(17-81)apps/web/utils/calendar/availability-types.ts (1)
BusyPeriod(1-4)apps/web/utils/calendar/providers/google-availability.ts (1)
googleAvailabilityProvider(63-89)apps/web/utils/calendar/providers/microsoft-availability.ts (1)
microsoftAvailabilityProvider(65-91)
apps/web/__tests__/e2e/calendar/microsoft-calendar.test.ts (1)
apps/web/utils/calendar/providers/microsoft-availability.ts (1)
microsoftAvailabilityProvider(65-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Software Component Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
🔇 Additional comments (8)
apps/web/utils/outlook/calendar-client.ts (5)
1-11: LGTM!The imports and logger setup follow the established patterns correctly.
13-23: LGTM!The CalendarAuthProvider implementation correctly implements the AuthenticationProvider interface required by Microsoft Graph Client.
25-42: LGTM!The OAuth2 URL generation correctly constructs the Microsoft authorization endpoint with proper parameters.
44-135: Well-structured token refresh flow.The overall token refresh implementation correctly handles the Microsoft OAuth2 flow, including token validation, refresh, and persistence. The error handling for
invalid_grantis appropriate.
158-193: LGTM!The token persistence logic correctly handles optional fields and properly converts timestamp units from seconds to milliseconds for the Date object.
apps/web/__tests__/e2e/calendar/google-calendar.test.ts (1)
11-105: Overall e2e flow and safeguards look solidConditional execution via environment flags, defensive checks for missing account/connection/tokens, and the use of a real provider to exercise the full stack all look well thought out and safe to run only when explicitly configured.
apps/web/__tests__/e2e/calendar/microsoft-calendar.test.ts (1)
11-108: Symmetric Outlook e2e wiring looks goodThe setup/guard clauses, database queries, and provider invocation mirror the Google flow cleanly and give you a good end‑to‑end signal when a Microsoft calendar is connected.
apps/web/utils/calendar/unified-availability.ts (1)
14-143: Unified availability aggregation and error isolation look correctThe function correctly:
- Computes a single time window in the requested timezone.
- Loads all connected calendar connections with only enabled calendars.
- Fans out per connection, isolating failures by catching and returning empty arrays.
- Flattens results and traces both request metadata and aggregate counts.
This is a clean, composable abstraction for multi‑provider availability.
| if (!testEmail) { | ||
| console.warn("\n⚠️ Set TEST_OUTLOOK_EMAIL env var to run these tests"); | ||
| console.warn( | ||
| " Example: TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e outlook-calendar\n", | ||
| ); |
There was a problem hiding this comment.
Align example command with documented usage to avoid confusion
The usage block at the top shows pnpm test-e2e microsoft-calendar, but the warning example uses outlook-calendar. That mismatch can confuse anyone trying to run the test.
You can align them like this:
- console.warn(
- " Example: TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e outlook-calendar\n",
- );
+ console.warn(
+ " Example: TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e microsoft-calendar\n",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!testEmail) { | |
| console.warn("\n⚠️ Set TEST_OUTLOOK_EMAIL env var to run these tests"); | |
| console.warn( | |
| " Example: TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e outlook-calendar\n", | |
| ); | |
| if (!testEmail) { | |
| console.warn("\n⚠️ Set TEST_OUTLOOK_EMAIL env var to run these tests"); | |
| console.warn( | |
| " Example: TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e microsoft-calendar\n", | |
| ); |
🤖 Prompt for AI Agents
In apps/web/__tests__/e2e/calendar/microsoft-calendar.test.ts around lines 37 to
41, the warning example command uses "outlook-calendar" which contradicts the
documented usage "microsoft-calendar"; update the warning to show the same
command (e.g., TEST_OUTLOOK_EMAIL=your@email.com pnpm test-e2e
microsoft-calendar) so the example matches the documented test name and avoids
confusion.
| if (!refreshToken) throw new SafeError("No refresh token"); | ||
|
|
||
| // Check if token is still valid | ||
| if (expiresAt && expiresAt > Date.now() && accessToken) { |
There was a problem hiding this comment.
Critical: Token expiry comparison has unit mismatch.
The comparison expiresAt > Date.now() will always fail because Date.now() returns milliseconds while expiresAt is stored in seconds (as evident from line 110). This causes tokens to be considered expired on every call, forcing unnecessary refresh requests.
Apply this diff to fix the comparison:
- if (expiresAt && expiresAt > Date.now() && accessToken) {
+ if (expiresAt && expiresAt * 1000 > Date.now() && accessToken) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (expiresAt && expiresAt > Date.now() && accessToken) { | |
| if (expiresAt && expiresAt * 1000 > Date.now() && accessToken) { |
🤖 Prompt for AI Agents
In apps/web/utils/outlook/calendar-client.ts around line 58, the token expiry
comparison uses expiresAt (seconds) against Date.now() (milliseconds), causing
tokens to be treated as expired; fix by normalizing units before comparing —
either convert expiresAt to milliseconds (expiresAt * 1000) and compare to
Date.now(), or convert Date.now() to seconds (Math.floor(Date.now() / 1000)) and
compare to expiresAt, so the if condition correctly detects unexpired tokens.
| export async function fetchMicrosoftCalendars(calendarClient: Client): Promise< | ||
| Array<{ | ||
| id?: string; | ||
| name?: string; | ||
| description?: string; | ||
| timeZone?: string; | ||
| }> | ||
| > { | ||
| try { | ||
| const response = await calendarClient | ||
| .api("/me/calendars") | ||
| .select("id,name,color,isDefaultCalendar,canEdit,owner,timeZone") | ||
| .get(); | ||
|
|
||
| return response.value || []; | ||
| } catch (error) { | ||
| logger.error("Error fetching Microsoft calendars", { error }); | ||
| throw new SafeError("Failed to fetch calendars"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix mismatch between selected fields and return type.
The function selects fields that don't match the declared return type:
- Selected but not in return type:
color,isDefaultCalendar,canEdit,owner - In return type but not selected:
description
This means description will always be undefined, and extra fields are fetched unnecessarily.
Apply this diff to align the select with the return type:
const response = await calendarClient
.api("/me/calendars")
- .select("id,name,color,isDefaultCalendar,canEdit,owner,timeZone")
+ .select("id,name,description,timeZone")
.get();Alternatively, if you need the additional fields, update the return type accordingly.
🤖 Prompt for AI Agents
In apps/web/utils/outlook/calendar-client.ts around lines 137 to 156, the
selected fields from the Microsoft Graph call don’t match the declared return
type: the select includes color, isDefaultCalendar, canEdit, owner (which aren’t
in the return type) and omits description (which is in the return type). Fix by
making the select and return type consistent — either add description to the
.select(...) and remove any unneeded fields from the select so only
id,name,description,timeZone are requested, or expand the function’s return type
to include color, isDefaultCalendar, canEdit, and owner if those fields are
required; update the .select(...) and the returned object type accordingly and
keep the logger/error behavior unchanged.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files (reviewed changes from recent commits).
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="apps/web/__tests__/e2e/calendar/google-calendar.test.ts">
<violation number="1" location="apps/web/__tests__/e2e/calendar/google-calendar.test.ts:170">
The test unconditionally expects at least one busy period even though an empty calendar is a valid outcome, so the E2E will fail whenever no events exist for the checked date. Remove or gate this assertion so the test only verifies structure unless you explicitly seed a busy period.</violation>
</file>
<file name="apps/web/utils/calendar/providers/microsoft-availability.ts">
<violation number="1" location="apps/web/utils/calendar/providers/microsoft-availability.ts:50">
Swallowing calendar API errors here makes the provider return an empty busy list when every request fails, so scheduling proceeds with incorrect availability instead of surfacing the failure.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| expect(Array.isArray(busyPeriods)).toBe(true); | ||
|
|
||
| // Verify at least one busy period was found | ||
| expect(busyPeriods.length).toBeGreaterThan(0); |
There was a problem hiding this comment.
The test unconditionally expects at least one busy period even though an empty calendar is a valid outcome, so the E2E will fail whenever no events exist for the checked date. Remove or gate this assertion so the test only verifies structure unless you explicitly seed a busy period.
Prompt for AI agents
Address the following comment on apps/web/__tests__/e2e/calendar/google-calendar.test.ts at line 170:
<comment>The test unconditionally expects at least one busy period even though an empty calendar is a valid outcome, so the E2E will fail whenever no events exist for the checked date. Remove or gate this assertion so the test only verifies structure unless you explicitly seed a busy period.</comment>
<file context>
@@ -0,0 +1,181 @@
+ expect(Array.isArray(busyPeriods)).toBe(true);
+
+ // Verify at least one busy period was found
+ expect(busyPeriods.length).toBeGreaterThan(0);
+
+ // Verify busy periods have correct structure
</file context>
| } | ||
| } | ||
| } | ||
| } catch (calendarError) { |
There was a problem hiding this comment.
Swallowing calendar API errors here makes the provider return an empty busy list when every request fails, so scheduling proceeds with incorrect availability instead of surfacing the failure.
Prompt for AI agents
Address the following comment on apps/web/utils/calendar/providers/microsoft-availability.ts at line 50:
<comment>Swallowing calendar API errors here makes the provider return an empty busy list when every request fails, so scheduling proceeds with incorrect availability instead of surfacing the failure.</comment>
<file context>
@@ -20,45 +20,42 @@ async function fetchMicrosoftCalendarBusyPeriods({
}
}
}
+ } catch (calendarError) {
+ logger.error("Error fetching calendar events", {
+ calendarId,
</file context>
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation