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.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds Microsoft Calendar support, provider abstractions, unified availability across Google+Microsoft, centralized calendar OAuth handling, calendar settings (timezone & booking link) with UI and DB migration, test scaffolding for calendar availability, and updates OAuth redirect base to env.NEXT_PUBLIC_BASE_URL. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Server
participant Microsoft
participant DB
rect rgba(221,235,247,0.8)
Note over User,Microsoft: Connect Microsoft Calendar
User->>Browser: Click "Connect Microsoft Calendar"
Browser->>Server: GET /api/outlook/calendar/auth-url
Server->>Server: create OAuth state, set cookie
Server-->>Browser: { url }
Browser->>Microsoft: Redirect to Microsoft OAuth
Microsoft-->>Browser: Consent -> redirect with code
Browser->>Server: GET /api/outlook/calendar/callback?code=...
Server->>Server: handleCalendarCallback(request, microsoftCalendarProvider)
Server->>Microsoft: exchange code for tokens
Server->>DB: create calendar connection
Server->>Server: microsoftCalendarProvider.syncCalendars(...) -> upsert calendars
Server-->>Browser: Redirect back to app with status
end
sequenceDiagram
participant Client
participant Server
participant Google
participant Microsoft
participant DB
rect rgba(237,247,224,0.8)
Note over Client,DB: Unified availability request
Client->>Server: Request availability (start,end,timezone)
Server->>DB: fetch enabled calendar connections for account
Server->>Server: group connections by provider
par Google
Server->>Google: googleAvailabilityProvider.fetchBusyPeriods(...)
and Microsoft
Server->>Microsoft: microsoftAvailabilityProvider.fetchBusyPeriods(...)
end
Server->>Server: flatten busy periods, convert to target timezone
Server-->>Client: return BusyPeriod[] (unified)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (1)
111-114: Update the comment to reflect new behavior.The comment states the form will "only show if we have an AI rule," but the code below (lines 257, 274) now shows the Custom button and form in test mode regardless of
hasAiRules. The alert at lines 276-283 handles the no-rules scenario instead.Apply this diff:
- // only show test rules form if we have an AI rule. this form won't match group/static rules which will confuse users + // Check if AI rules exist to show a warning in the custom test form. + // The custom form won't match group/static rules. const hasAiRules = rules?.some( (rule) => isAIRule(rule) && !isGroupRule(rule) && !isStaticRule(rule), );
🧹 Nitpick comments (13)
apps/web/app/(app)/no-access/page.tsx (1)
12-33: Clean implementation with clear user guidance.The page provides a user-friendly error state with a clear message and actionable next step. The component structure, responsive layout, and UI composition are well done.
Consider these optional enhancements:
- Add page metadata for better SEO and browser tab display:
+export const metadata = { + title: "No Access", + description: "Email account not found or you don't have access to it", +}; + export default function NoAccessPage() {
- Add aria-label to the icon for better accessibility:
- <AlertCircle className="h-5 w-5 text-destructive" /> + <AlertCircle className="h-5 w-5 text-destructive" aria-label="Error" />apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (1)
274-286: UX improvement: Alert provides guidance instead of hiding functionality.The conditional logic is sound, and the alert clearly informs users when AI rules are needed. The
space-y-2spacing accommodates the alert nicely.Consider using a less alarming variant:
- {!hasAiRules && ( - <Alert variant="destructive"> + {!hasAiRules && ( + <Alert> <AlertDescription>The
destructivevariant (typically red) feels harsh for informational guidance. The default alert styling may be more appropriate for this advisory message.apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnectionCard.tsx (1)
31-46: Consider stronger typing for the provider parameter.The
getProviderInfohelper currently accepts any string and falls back to Google. While this provides backward compatibility, it could silently hide issues if an unexpected provider value is used.Consider adding a type guard or using a union type:
+type CalendarProvider = "microsoft" | "google"; + -const getProviderInfo = (provider: string) => { +const getProviderInfo = (provider: string): typeof providers[CalendarProvider] => { 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", }, - }; + } as const satisfies Record<CalendarProvider, { name: string; icon: string; alt: string }>; - return providers[provider as keyof typeof providers] || providers.google; + if (provider in providers) { + return providers[provider as CalendarProvider]; + } + console.warn(`Unknown calendar provider: ${provider}, falling back to Google`); + return providers.google; };This makes the fallback behavior more explicit and helps catch unexpected provider values during development.
apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (1)
18-74: Consider refactoring to reduce code duplication.The
handleConnectGoogleandhandleConnectMicrosoftfunctions are nearly identical, differing only in the API endpoint and provider name. This duplication could lead to maintenance issues.Consider extracting a shared handler:
const handleConnect = async ( provider: "google" | "microsoft", setLoading: (loading: boolean) => void, ) => { const endpoints = { google: "/api/google/calendar/auth-url", microsoft: "/api/outlook/calendar/auth-url", }; const providerNames = { google: "Google", microsoft: "Microsoft", }; setLoading(true); try { const response = await fetchWithAccount({ url: endpoints[provider], emailAccountId, init: { headers: { "Content-Type": "application/json" } }, }); if (!response.ok) { throw new Error(`Failed to initiate ${providerNames[provider]} calendar connection`); } const data: GetCalendarAuthUrlResponse = await response.json(); window.location.href = data.url; } catch (error) { logger.error(`Error initiating ${providerNames[provider]} calendar connection`, { error, emailAccountId, provider, }); toastError({ title: `Error initiating ${providerNames[provider]} calendar connection`, description: "Please try again or contact support", }); setLoading(false); } }; const handleConnectGoogle = () => handleConnect("google", setIsConnectingGoogle); const handleConnectMicrosoft = () => handleConnect("microsoft", setIsConnectingMicrosoft);apps/web/utils/outlook/scopes.ts (1)
20-28: Consider extracting common scopes to reduce duplication.The
CALENDAR_SCOPESduplicates five base scopes fromSCOPES("openid", "profile", "email", "User.Read", "offline_access"). While this duplication may be intentional for independent OAuth flows, consider extracting common scopes to a shared constant to improve maintainability.Example refactor:
+const BASE_OAUTH_SCOPES = [ + "openid", + "profile", + "email", + "User.Read", + "offline_access", +] as const; + -export const SCOPES = [ - "openid", - "profile", - "email", - "User.Read", - "offline_access", // Required for refresh tokens +export const SCOPES = [ + ...BASE_OAUTH_SCOPES, "Mail.ReadWrite", "Mail.Send", "Mail.ReadBasic", "Mail.Read", "Mail.Read.Shared", "MailboxSettings.ReadWrite", ...(env.NEXT_PUBLIC_CONTACTS_ENABLED ? ["Contacts.ReadWrite"] : []), ] as const; -export const CALENDAR_SCOPES = [ - "openid", - "profile", - "email", - "User.Read", - "offline_access", // Required for refresh tokens +export const CALENDAR_SCOPES = [ + ...BASE_OAUTH_SCOPES, "Calendars.Read", "Calendars.ReadWrite", ] as const;apps/web/utils/calendar/availability-types.ts (1)
1-20: Types look solid; consider centralizing provider/expiry semantics
BusyPeriodandCalendarAvailabilityProviderare straightforward and align with how the rest of the code appears to use them. Two small consistency tweaks you may want to consider:
- Extract a shared
CalendarProviderName = "google" | "microsoft"type and reuse it across availability, OAuth providers, and helpers to avoid drift.- Document (or narrow) the
expiresAt: number | nullparameter to make its units explicit (e.g. ms since epoch) so it stays in sync with howCalendarTokens.expiresAt: Date | nullis derived and converted elsewhere.apps/web/utils/calendar/providers/microsoft.ts (1)
15-78: Token exchange + profile fetch look correct; minor hardening possibleThe overall flow (code → token endpoint →
/me→ email + refresh token checks) looks good and matches Microsoft’s expectations.You might want to tighten a couple of edges:
- Guard explicitly on
tokens.access_tokenbefore using it, so failures surface as a clearer error than a later 401 from Graph.- When
tokenResponse.okorprofileResponse.okis false, consider logging vialogger.errorwith only non-sensitive fields (status, error codes/description) to make debugging easier while still avoiding access/refresh token leakage.apps/web/utils/calendar/oauth-types.ts (1)
1-40: Interfaces are coherent; consider a shared provider name aliasThese OAuth types line up well with how the providers and callback helpers are using them.
To reduce duplication and keep things in sync, you might:
- Extract a
CalendarProviderName = "google" | "microsoft"type here and use it for:
CalendarOAuthProvider.name,- The
providerparameters in other helpers (e.g.checkExistingConnection,createCalendarConnection),CalendarAvailabilityProvider.name.That makes it harder for a new provider string or typo to slip into only one part of the system.
apps/web/utils/outlook/calendar-client.ts (2)
44-135: Token refresh flow is sound; clarify expiry units and error signalingThe logic around:
- Reusing a still-valid access token when
expiresAt > Date.now()and- Refreshing tokens and persisting them via
saveCalendarTokensis consistent and avoids unnecessary refresh calls.
Two small improvements to consider:
- Explicitly document that the
expiresAtparameter is expected to be milliseconds since epoch (whiletokens.expires_atinsaveCalendarTokensis seconds). This avoids confusion with other parts of the codebase that also deal in expiry timestamps.- When the token endpoint returns
invalid_grant, you’re already logging a warning. You might additionally wrap that case in aSafeError(or a dedicated error subtype) so callers likesyncCalendarscan clearly distinguish “credentials revoked/invalid” from transient network or Graph issues.
137-153: Calendar fetch wrapper is fine; consider pagination support
fetchMicrosoftCalendarsis a clean wrapper around/me/calendarswith appropriate error logging and a user-safe error message.If you expect users with many calendars, consider following the
@odata.nextLinkto collect all pages so the sync and availability layers see the full set rather than just the first page.apps/web/utils/calendar/oauth-callback-helpers.ts (3)
16-64: Callback validation is robust; origin handling is intentionally strictThe calendar-specific
validateOAuthCallbackdoes a nice job of:
- Enforcing origin against
NEXT_PUBLIC_BASE_URL,- Clearing the state cookie via response headers, and
- Handling missing/short codes and state mismatches via
RedirectError.One design choice to think about: on an origin mismatch (Lines 34–41) you currently throw a plain
Error, which will likely surface as a 500. If you’d prefer a more graceful UX, you could instead redirect with an explicit error code (e.g.error=invalid_origin) using the sameRedirectErrorpattern. If you want this to remain a hard failure for security reasons, the current behavior is fine.
69-94: State decoding + validation is well-structured
parseAndValidateCalendarStatecleanly separates concerns:
- Decoding the base64 state via
parseOAuthState, and- Validating structure with
calendarOAuthStateSchema, redirecting with a clear error when anything goes wrong.The generic
parseOAuthState<Omit<CalendarOAuthState, "nonce">>type argument isn’t strictly necessary since Zod dictates the final shape anyway, but it doesn’t cause issues. If you want to simplify, you could just parse tounknownand rely entirely on the Zod schema.
143-179: Connection helpers are fine; provider string could be centralized
checkExistingConnectionandcreateCalendarConnectionare straightforward Prisma helpers and correctly mark new connections asisConnected: true.As with other files, you may eventually want to replace the inline
"google" | "microsoft"union with a sharedCalendarProviderNametype to keep all usages synchronized as you add more providers.
📜 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 (36)
README.md(1 hunks)apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx(3 hunks)apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx(3 hunks)apps/web/app/(app)/[emailAccountId]/assistant/TestCustomEmailForm.tsx(2 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/(app)/no-access/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/ai/choose-rule/run-rules.ts(1 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/email-account.ts(2 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)apps/web/utils/reply-tracker/handle-conversation-status.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/utils/email-account.tsapps/web/app/(app)/no-access/page.tsxapps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx
🧬 Code graph analysis (23)
apps/web/utils/calendar/providers/google.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/utils/calendar/client.ts (2)
getCalendarOAuth2Client(31-37)fetchGoogleCalendars(117-127)apps/web/utils/outlook/calendar-client.ts (1)
getCalendarClientWithRefresh(44-135)
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/app/(app)/[emailAccountId]/assistant/TestCustomEmailForm.tsx (1)
apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx (1)
ResultsDisplay(23-64)
apps/web/utils/oauth/account-linking.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/utils/ai/calendar/availability.ts (1)
apps/web/utils/calendar/unified-availability.ts (1)
getUnifiedCalendarAvailability(14-137)
apps/web/utils/calendar/providers/microsoft.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/outlook/calendar-client.ts (2)
getCalendarClientWithRefresh(44-135)fetchMicrosoftCalendars(137-153)
apps/web/utils/oauth/callback-validation.ts (1)
apps/web/env.ts (1)
env(16-244)
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/app/api/outlook/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/(app)/no-access/page.tsx (1)
apps/web/components/ui/card.tsx (2)
CardTitle(141-141)CardDescription(142-142)
apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx (1)
apps/web/utils/ai/choose-rule/run-rules.ts (1)
RunRulesResult(44-63)
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/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/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/utils/calendar/oauth-callback-helpers.ts (7)
apps/web/utils/oauth/callback-validation.ts (1)
validateOAuthCallback(25-85)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/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/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/app/api/google/linking/callback/route.ts (1)
apps/web/env.ts (1)
env(16-244)
apps/web/app/api/mcp/[integration]/callback/route.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/utils/reply-tracker/handle-conversation-status.ts (3)
apps/web/utils/types.ts (2)
RuleWithActions(16-18)ParsedMessage(51-73)apps/web/utils/llms/types.ts (1)
EmailAccountWithAI(10-30)apps/web/utils/llms/model.ts (1)
ModelType(17-17)
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)
⏰ 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 (46)
apps/web/utils/email-account.ts (1)
1-1: LGTM! User-friendly redirect improves UX.Replacing the error throw with a redirect to
/no-accessis a good improvement. Theredirect()function from Next.js 15 correctly handles server-side navigation by throwing aNEXT_REDIRECTerror internally, which is the recommended pattern for Server Components, Route Handlers, and Server Actions.Also applies to: 19-21
apps/web/utils/ai/choose-rule/run-rules.ts (1)
153-153: LGTM! Proper test flag propagation.The
isTestflag is correctly forwarded to enable test-aware conversation status handling, allowing tests to bypass external API calls.apps/web/utils/reply-tracker/handle-conversation-status.ts (1)
25-25: Well-designed test mode implementation.The
isTestparameter enables unit testing by bypassing the external API call to fetch thread messages. Key aspects:
- Default value
falsemaintains backward compatibility- When
true, uses the provided message as a single-entry thread, avoiding real API calls- Logging helps with debugging test vs. production flows
- Downstream logic (sorting, user detection, AI status determination) handles single-message threads appropriately
This is a clean pattern for making the code testable without affecting production behavior.
Also applies to: 32-32, 40-40, 43-46
apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (2)
19-19: LGTM: Alert components imported for user guidance.The new imports support the informational alert shown when AI rules are absent.
257-266: Good change: Custom button now always visible in test mode.Removing the
hasAiRulescheck improves discoverability. Users can access the custom form even without AI rules, and the alert below provides appropriate guidance.apps/web/app/(app)/[emailAccountId]/assistant/TestCustomEmailForm.tsx (1)
65-72: LGTM! Improved UI presentation for test results.The Card wrapper with header provides better visual structure for test results, and the
showFullContent={true}prop correctly enables the full-content rendering mode introduced in ResultDisplay.tsx.apps/web/app/(app)/[emailAccountId]/calendars/ConnectCalendar.tsx (1)
77-109: LGTM! Good loading state and accessibility.The button implementation correctly:
- Disables both buttons when either connection is in progress (preventing race conditions)
- Provides clear visual feedback with "Connecting..." text
- Uses semantic button text and icons for accessibility
apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx (1)
23-81: LGTM! Clean implementation of showFullContent feature.The implementation:
- Maintains backward compatibility with
showFullContent = falsedefault- Cleanly separates the two rendering modes (full content vs. compact HoverCard)
- Properly adjusts layout classes based on the display mode
- Uses minimal prop drilling, which is appropriate for this component hierarchy
apps/web/app/(app)/[emailAccountId]/calendars/CalendarConnections.tsx (1)
17-17: LGTM! Text updated to reflect Microsoft Calendar support.The empty-state message now accurately reflects that both Google and Microsoft Calendar connections are supported.
package.json (2)
24-24: Based on verification, pnpm 10.22.0 is the latest stable release (confirmed via npm registry). Lifecycle scripts of dependencies are not executed during installation by default, which is a breaking change aimed at increasing security. pnpm 10 introduces several notable updates, including enhanced security by adopting SHA256 for hashing algorithms. Other breaking changes include modified behavior for thepnpm linkcommand and removal of lockfile v6-to-v9 conversion. No security vulnerabilities were found in pnpm 10.22.0 itself. If upgrading from pnpm 9.x, review the documented breaking changes carefully for potential impact on your project.
21-21: turbo 2.6.1 is safe to upgrade.Turbo v2.6.1 contains only bug fixes and small improvements with no breaking changes, and no security advisories were found. The update is ready for deployment.
apps/web/app/api/mcp/[integration]/callback/route.ts (1)
53-53: Code change is correct and well-documented.Verification confirms:
NEXT_PUBLIC_BASE_URLis documented in.env.examplewith the example valuehttp://localhost:3000- README explicitly documents the variable: "The URL where your app is hosted (e.g.,
http://localhost:3000for local development orhttps://yourdomain.comfor production)"- Docker-specific guidance is provided, noting that
NEXT_PUBLIC_BASE_URLmust be specified as a build argument and cannot be changed at runtime- Consistent usage across all OAuth flows (Gmail, Calendar, MCP integrations)
The centralization of redirect URLs to environment configuration is properly documented and supported. Developers have clear guidance on configuration for all deployment scenarios.
apps/web/app/api/google/linking/callback/route.ts (1)
46-46: LGTM: Consistent migration to environment-based redirect URLs.All redirect URLs now consistently use
env.NEXT_PUBLIC_BASE_URLinstead of request-derived origins. This provides a stable, configurable base URL across all OAuth callback scenarios (cached results, concurrent processing, success/error paths).Also applies to: 60-60, 203-203, 241-241, 250-250
README.md (1)
414-431: LGTM: Clear Microsoft Calendar integration documentation.The documentation clearly outlines the Microsoft Calendar setup process with appropriate permissions (Calendars.Read, Calendars.ReadWrite) and notes that the calendar OAuth flow is separate from the main email flow.
apps/web/app/api/outlook/calendar/callback/route.ts (1)
1-10: LGTM: Clean delegation pattern for Outlook calendar callback.The implementation follows established patterns and properly delegates to the shared
handleCalendarCallbackwith the Microsoft-specific provider. The scoped logger and error wrapper are correctly applied.apps/web/app/(app)/[emailAccountId]/calendars/page.tsx (1)
9-14: LGTM: Improved responsive layout.The updated layout classes and component swap from
ConnectCalendarButtontoConnectCalendarprovide better responsive behavior, stacking elements on small screens and displaying them horizontally on larger screens.apps/web/utils/oauth/account-linking.ts (1)
2-2: LGTM: Successfully migrated to environment-based redirects.The removal of the
baseUrlparameter and adoption ofenv.NEXT_PUBLIC_BASE_URLaligns with the PR's goal of centralizing redirect URL configuration. All redirect scenarios correctly use the environment variable.Also applies to: 30-30
apps/web/utils/calendar/providers/microsoft-availability.ts (2)
44-48: Verify business logic for tentative events.The implementation treats all non-free statuses (including "tentative") as busy periods. While this is conservative and prevents double-booking, tentative events typically indicate unconfirmed appointments where the user might still be available. Verify this behavior aligns with user expectations and business requirements.
Consider whether users should be marked as available during tentative events, or if this conservative approach is preferred.
11-66: LGTM: Well-structured Microsoft Calendar availability provider.The implementation properly:
- Queries the Microsoft Graph getSchedule API
- Converts schedule items to BusyPeriod objects
- Includes appropriate trace logging and error handling
- Follows the CalendarAvailabilityProvider interface
Also applies to: 68-94
apps/web/app/api/outlook/linking/callback/route.ts (1)
49-49: LGTM: Consistent migration to environment-based redirect URLs.All redirect URLs and the OAuth token exchange redirect_uri now consistently use
env.NEXT_PUBLIC_BASE_URL. This matches the pattern applied to the Google linking callback and provides stable, configurable URLs across all scenarios.Also applies to: 63-63, 83-83, 274-274, 310-310, 319-319
apps/web/utils/calendar/providers/google-availability.ts (3)
1-10: LGTM!The imports and logger setup are well-structured and follow the established patterns in the codebase.
11-61: LGTM!The busy periods fetching logic is well-implemented with proper error handling and logging. The function correctly aggregates busy periods across multiple calendars.
63-89: LGTM!The provider implementation correctly follows the CalendarAvailabilityProvider interface and delegates appropriately to the helper function. Token refresh is properly handled.
apps/web/utils/oauth/callback-validation.ts (1)
2-2: LGTM!The migration to use
env.NEXT_PUBLIC_BASE_URLinstead of a parameter-based approach is a good improvement. This ensures consistent redirect URL construction across the application and aligns with environment-based configuration best practices.Also applies to: 25-33
apps/web/utils/ai/calendar/availability.ts (2)
6-6: LGTM!The import update and enhanced tool description correctly reflect the unified calendar availability approach that now supports both Google and Microsoft calendars.
Also applies to: 112-113
126-142: LGTM!The unified availability approach with proper error handling is a significant improvement. The fallback to an empty
busyPeriodsarray ensures graceful degradation when calendar access fails.apps/web/utils/calendar/handle-calendar-callback.ts (4)
1-24: LGTM!The imports are well-organized and the function signature with JSDoc documentation clearly describes the unified handler's purpose. The type-safe approach using the CalendarOAuthProvider abstraction is excellent.
25-60: LGTM!The validation and verification steps are well-structured with proper security checks. The delegation to helper functions keeps the code clean and maintainable.
62-116: LGTM!The token exchange and calendar synchronization flow is well-implemented. The check for existing connections prevents duplicates, and the provider abstraction allows for clean extensibility across Google and Microsoft providers.
117-137: LGTM!The error handling is robust with specific handling for RedirectError and a fallback mechanism for unexpected errors. This ensures users are never left without a proper redirect.
apps/web/app/api/outlook/calendar/auth-url/route.ts (2)
1-21: LGTM!The helper function cleanly generates the OAuth state and URL. The type distinction ("calendar") allows for proper state validation during the callback.
23-37: LGTM!The route handler is properly secured with
withEmailAccountmiddleware and follows the OAuth state cookie pattern consistently. The 10-minute cookie expiry is appropriate for the OAuth flow.apps/web/utils/calendar/providers/google.ts (3)
1-14: LGTM!The imports and provider setup follow established patterns. The logger scope clearly identifies this as the Google calendar provider.
16-46: LGTM!The token exchange logic is secure with proper validation of the ID token and all required fields. The error messages are clear and help with debugging.
48-98: LGTM!The calendar synchronization logic is well-implemented with proper error handling. Using
upsertensures idempotency, and marking the connection as disconnected on error is the correct behavior for maintaining data consistency.apps/web/utils/calendar/unified-availability.ts (4)
1-10: LGTM!The imports are well-organized, and the use of
@date-fns/tzfor timezone handling is appropriate for calendar availability queries.
11-60: LGTM!The timezone-aware date handling using
TZDateandstartOfDay/endOfDayensures accurate availability queries across different timezones. The filtering for connected and enabled calendars is correct.
62-122: LGTM!The provider-specific fetching logic is well-structured with proper error isolation. Each connection's errors are caught independently, ensuring that one failing connection doesn't prevent fetching availability from others.
124-137: LGTM!The result aggregation properly uses
Promise.allfor concurrent fetching and flattens the results into a unified list. The summary logging provides useful debugging information about the provider breakdown.apps/web/app/api/google/calendar/callback/route.ts (1)
1-10: LGTM!The refactoring to delegate to
handleCalendarCallbackis an excellent improvement. This eliminates code duplication and makes the codebase significantly more maintainable by centralizing the OAuth callback logic.apps/web/utils/outlook/calendar-client.ts (2)
25-42: OAuth2 URL construction looks correctThe authorize URL generation (client_id, scopes including
offline_access, and redirect URI based onNEXT_PUBLIC_BASE_URL) looks consistent with the callback handler and should work fine as-is.
155-190: Token persistence helper is straightforward and safe
saveCalendarTokenscorrectly bails out when there’s noaccess_token, converts seconds to aDateforexpiresAt, and logs both success and failure with the connectionId. No issues here.apps/web/utils/calendar/oauth-callback-helpers.ts (4)
99-104: Redirect URL builder matches the per-account routing
buildCalendarRedirectUrlcorrectly constructs a URL rooted atNEXT_PUBLIC_BASE_URLwith the emailAccountId-prefixed/calendarspath viaprefixPath, which keeps routing concerns localized.
109-138: Access check against emailAccount is appropriate
verifyEmailAccountAccessdoes the right thing by:
- Requiring a session with
user.id, and- Ensuring the
emailAccountIdbelongs to that user before proceeding,with distinct
unauthorizedvsforbiddenerrors communicated via redirects. This is a solid guardrail around the callback.
184-203: Redirect helpers correctly preserve headers (including cookie changes)Both
redirectWithMessageandredirectWithErrortweak the query string and then redirect while reusing the previously preparedresponseHeaders, so state-cookie deletions and other header mutations survive through the final response. This matches howRedirectErroris used upstream.
208-217: RedirectError encapsulation is clean
RedirectErrorcarrying both theredirectUrlandresponseHeadersis a neat pattern for wiring redirects through higher-level orchestration code without losing cookie/header changes. No issues here.
| async syncCalendars( | ||
| connectionId: string, | ||
| accessToken: string, | ||
| refreshToken: string, | ||
| emailAccountId: string, | ||
| expiresAt: Date | null, | ||
| ): Promise<void> { | ||
| try { | ||
| const calendarClient = await getCalendarClientWithRefresh({ | ||
| accessToken, | ||
| refreshToken, | ||
| expiresAt: expiresAt?.getTime() ?? null, | ||
| emailAccountId, | ||
| }); | ||
|
|
||
| const microsoftCalendars = await fetchMicrosoftCalendars(calendarClient); | ||
|
|
||
| for (const microsoftCalendar of microsoftCalendars) { | ||
| if (!microsoftCalendar.id) continue; | ||
|
|
||
| await prisma.calendar.upsert({ | ||
| where: { | ||
| connectionId_calendarId: { | ||
| connectionId, | ||
| calendarId: microsoftCalendar.id, | ||
| }, | ||
| }, | ||
| update: { | ||
| name: microsoftCalendar.name || "Untitled Calendar", | ||
| description: microsoftCalendar.description, | ||
| timezone: microsoftCalendar.timeZone, | ||
| }, | ||
| create: { | ||
| connectionId, | ||
| calendarId: microsoftCalendar.id, | ||
| name: microsoftCalendar.name || "Untitled Calendar", | ||
| description: microsoftCalendar.description, | ||
| timezone: microsoftCalendar.timeZone, | ||
| isEnabled: true, | ||
| }, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error("Error syncing calendars", { error, connectionId }); | ||
| await prisma.calendarConnection.update({ | ||
| where: { id: connectionId }, | ||
| data: { isConnected: false }, | ||
| }); | ||
| throw error; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Avoid marking connections disconnected on every sync error
Right now any error in syncCalendars (invalid tokens, Graph outages, network flakiness, transient DB issues) results in:
- Logging the error, and
- Forcing
calendarConnection.isConnected = false(Lines 123–127),
which will push users back through a re-link flow even for transient problems.
Consider distinguishing between credential/consent issues and transient failures, and only flipping isConnected to false for the former. For example, you could:
- Have
getCalendarClientWithRefreshthrow a typed error (orSafeErrorwith a specific code) for invalid_grant / auth failures, and - In this catch block, only update
isConnectedwhen that specific error type/code is detected, while rethrowing other errors without modifying the connection.
This keeps the current behavior for genuinely broken connections but avoids unnecessary disconnects when the provider or DB is temporarily unavailable.
🤖 Prompt for AI Agents
In apps/web/utils/calendar/providers/microsoft.ts around lines 80–130, the catch
block unconditionally marks the calendar connection as disconnected on any sync
error; change it to only set calendarConnection.isConnected = false for explicit
authentication/consent errors (e.g. an error type, class or a specific
error.code like "invalid_grant" or "auth_failure") while leaving
transient/network/DB errors untouched. Make getCalendarClientWithRefresh throw a
typed error or attach a stable code/property for auth failures, then in this
catch inspect error instanceof AuthError or error.code ===
"invalid_grant"/"auth_failure" (or similar) and only call
prisma.calendarConnection.update(...) in that branch; for all other errors just
log and rethrow without flipping isConnected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/reply-tracker/generate-draft.ts (1)
109-114: Consider bypassing cache in test mode.The Redis cache check runs even when testing, which means test mode may return a previously cached draft instead of generating a fresh one based on current rules and knowledge. This could make test results misleading if rules or context have changed since the cache entry was created.
Should test mode bypass the cache to ensure it always reflects the current system state? Consider adding a condition like:
if (reply && !testMode) return reply;However, you'd need to thread the test mode flag through to
generateDraftContent, or alternatively check ifpreviousConversationMessagesis intentionallynullas a signal of test mode (though that's more fragile).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx(4 hunks)apps/web/utils/ai/choose-rule/choose-args.ts(2 hunks)apps/web/utils/ai/choose-rule/run-rules.ts(2 hunks)apps/web/utils/reply-tracker/generate-draft.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/utils/ai/choose-rule/run-rules.ts
- apps/web/app/(app)/[emailAccountId]/assistant/ResultDisplay.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/utils/ai/choose-rule/choose-args.ts (2)
apps/web/utils/types.ts (1)
ParsedMessage(51-73)apps/web/utils/reply-tracker/generate-draft.ts (1)
fetchMessagesAndGenerateDraft(26-74)
apps/web/utils/reply-tracker/generate-draft.ts (1)
apps/web/utils/types.ts (1)
ParsedMessage(51-73)
⏰ 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). (3)
- GitHub Check: Software Component Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
🔇 Additional comments (4)
apps/web/utils/reply-tracker/generate-draft.ts (1)
30-34: Test mode bypasses message fetching as intended.The logic correctly short-circuits thread fetching when
testMessageis provided, using only the single test message and no previous conversation history. This aligns with the test-mode flow described in the PR.apps/web/utils/ai/choose-rule/choose-args.ts (3)
28-28: LGTM: Test mode parameter added with safe default.The
isTest = falsedefault ensures backward compatibility while enabling test mode when explicitly requested.
52-52: Good addition: Test mode is now logged.Including
isTestin the log helps with debugging and tracing test-mode executions.
59-59: LGTM: Test message passed correctly.When
isTestis true, the current message is passed as the test message tofetchMessagesAndGenerateDraft, which bypasses thread fetching and uses only the single test message. This correctly implements the test-mode flow.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 40 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="apps/web/app/api/google/linking/callback/route.ts">
<violation number="1" location="apps/web/app/api/google/linking/callback/route.ts:46">
Using env.NEXT_PUBLIC_BASE_URL for these redirects can break OAuth flows whenever the request host differs (e.g., preview URLs, local dev) or when the env var is missing, leading to failed redirects/500s. Please keep using request.nextUrl.origin (or an equivalent per-request origin) when building these URLs to preserve correct redirect behavior across environments.</violation>
</file>
<file name="apps/web/utils/calendar/providers/microsoft.ts">
<violation number="1" location="apps/web/utils/calendar/providers/microsoft.ts:28">
The token exchange request omits the `scope` form field. Microsoft Identity’s v2 /token endpoint requires `scope` for the authorization_code grant; without it, the request returns AADSTS900144 and the OAuth flow fails.</violation>
</file>
<file name="apps/web/utils/calendar/providers/google.ts">
<violation number="1" location="apps/web/utils/calendar/providers/google.ts:94">
Do not mark the calendar connection as disconnected for all sync errors. Only set isConnected=false for explicit authentication/consent failures; for transient/network/Graph/DB errors, log and rethrow without changing the connection state to avoid unnecessary re-link flows.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| logger.error("Error syncing calendars", { error, connectionId }); | ||
| await prisma.calendarConnection.update({ | ||
| where: { id: connectionId }, | ||
| data: { isConnected: false }, |
There was a problem hiding this comment.
Do not mark the calendar connection as disconnected for all sync errors. Only set isConnected=false for explicit authentication/consent failures; for transient/network/Graph/DB errors, log and rethrow without changing the connection state to avoid unnecessary re-link flows.
Prompt for AI agents
Address the following comment on apps/web/utils/calendar/providers/google.ts at line 94:
<comment>Do not mark the calendar connection as disconnected for all sync errors. Only set isConnected=false for explicit authentication/consent failures; for transient/network/Graph/DB errors, log and rethrow without changing the connection state to avoid unnecessary re-link flows.</comment>
<file context>
@@ -0,0 +1,99 @@
+ logger.error("Error syncing calendars", { error, connectionId });
+ await prisma.calendarConnection.update({
+ where: { id: connectionId },
+ data: { isConnected: false },
+ });
+ throw error;
</file context>
There was a problem hiding this comment.
2 issues found across 1 file (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/utils/actions/ai-rule.ts">
<violation number="1" location="apps/web/utils/actions/ai-rule.ts:141">
The generated test message ID now appends a trailing `}` (e.g. `testMessageId-...}`), which is a regression from the prior value and risks mismatches anywhere the ID is later compared. Please drop the extra brace.</violation>
<violation number="2" location="apps/web/utils/actions/ai-rule.ts:142">
The generated test thread ID now ends with a stray `}` (e.g. `testThreadId-...}`), which can cause mismatches when the thread ID is compared elsewhere. Remove the superfluous brace.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/utils/ai/calendar/availability.ts(7 hunks)apps/web/utils/ai/reply/draft-with-knowledge.ts(3 hunks)apps/web/utils/ai/reply/reply-context-collector.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/ai/calendar/availability.ts (2)
apps/web/utils/calendar/unified-availability.ts (1)
getUnifiedCalendarAvailability(14-149)apps/web/utils/llms/types.ts (1)
EmailAccountWithAI(10-32)
⏰ 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). (6)
- GitHub Check: Software Component Analysis Js
- GitHub Check: Static Code Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/web/utils/ai/reply/draft-with-knowledge.ts (4)
153-153: LGTM!The booking link context is correctly added to the prompt assembly in the appropriate location.
196-202: LGTM! Logging uses correct optional chaining pattern.The logging correctly uses
calendarAvailability.suggestedTimes?.length || 0with proper optional chaining. This is the correct pattern that should also be applied at line 115.
127-136: No issues found.The
calendarBookingLinkproperty is properly defined in theEmailAccountWithAItype atapps/web/utils/llms/types.tsline 18 and is correctly accessed at line 127 ofdraft-with-knowledge.ts. The property is consistently used throughout the codebase in user queries, database updates, and UI components.
106-125: Type verification confirmed—no issues found.The
CalendarAvailabilityContexttype includes all expected properties:
noAvailability?: boolean(defined asz.boolean().optional())suggestedTimes: Array<{ start: string; end: string }>(defined asz.array(timeSlotSchema))The code usage in lines 106-125 correctly accesses these properties.
apps/web/utils/ai/calendar/availability.ts (5)
14-31: LGTM! Well-structured schema with clear semantics.The
timeSlotSchemaand updatedschemawith thenoAvailabilityflag provide a clear structure for the AI to return availability results. The descriptions are helpful for guiding the AI's behavior, especially the inference of meeting duration from email context.
77-77: Excellent refactoring to unified availability model.The integration with
getUnifiedCalendarAvailabilitycleanly consolidates the per-calendar approach into a single unified query across all providers. The timezone handling withgetUserTimezoneensures consistent timezone context throughout the flow.The error handling pattern (returning empty
busyPeriodson failure) is consistent with the implementation inunified-availability.tsand allows graceful degradation rather than failing the entire request.Also applies to: 147-164
81-99: Clear and comprehensive system prompt.The updated prompt effectively communicates the unified availability model and timezone context. The examples and explicit instructions about non-overlapping busy periods are particularly valuable for guiding the AI's behavior correctly.
179-213: Well-implemented timezone resolution logic.The priority order is logical and clearly documented: user's explicit timezone → primary calendar timezone → any calendar timezone → UTC fallback. This ensures consistent timezone handling across the unified availability flow.
35-41: No issues found — callers correctly handle the new return type.Verification confirms that the single production caller in
apps/web/utils/reply-tracker/generate-draft.tsproperly handles the breaking change:
- Correctly receives
calendarAvailabilityfrom thePromise.alldestructuring- Passes it to
aiDraftWithKnowledgewith the proper type signature (CalendarAvailabilityContext | null)aiDraftWithKnowledgesafely accesses bothnoAvailabilityandsuggestedTimesproperties using optional chaining
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx (1)
53-61: Consider aligning null/undefined checks fortimezoneYou currently treat “no timezone” as
savedTimezone === nullin the effect, but use a generic falsy checkif (!data?.timezone) return null;in render. Iftimezonewere ever""orundefined, behavior between the effect and render could diverge. Consider tightening both to== nullchecks for consistency:- if (savedTimezone === null) { + if (savedTimezone == null) { executeUpdateTimezone({ timezone: currentTimezone }); return; } … - if (!data?.timezone) { + if (data?.timezone == null) { return null; }This avoids surprises if the backend ever returns
undefinedor an empty string.Also applies to: 89-91
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.test.ts (1)
164-289: Dismissal list tests are thorough; minor DRY opportunity on time mathThe
addDismissedPrompttests thoroughly cover empty/existing arrays, replacement behavior, and preserving unrelated entries while updating timestamps. Only nit: the repeated1000 * 60 * 60 * 24literals could be extracted to a sharedconst dayMs = 1000 * 60 * 60 * 24;to reduce duplication and make intent clearer, but the current code is still clear enough.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.test.ts(1 hunks)apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx(1 hunks)apps/web/app/(app)/[emailAccountId]/calendars/page.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(app)/[emailAccountId]/calendars/page.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.test.ts (1)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx (4)
shouldShowTimezonePrompt(127-149)DismissedPrompt(20-24)DISMISSAL_EXPIRY_DAYS(26-26)addDismissedPrompt(154-174)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx (3)
apps/web/providers/EmailAccountProvider.tsx (1)
useAccount(79-89)apps/web/hooks/useCalendars.ts (1)
useCalendars(4-6)apps/web/utils/actions/calendar.ts (1)
updateEmailAccountTimezoneAction(63-71)
⏰ 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 (3)
apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx (2)
20-88: Timezone detection flow and dismissal handling look solidThe overall flow (auto‑set when no timezone, then prompt only on mismatches with dismissal history in localStorage) is coherent and side‑effect boundaries are clear. The use of
dismissedPromptsin the effect dependency ensures the dialog won’t resurface for recently dismissed pairs while still reacting to future changes. No correctness issues stand out here.
124-174: Helper functions correctly encode expiry and replacement semantics
shouldShowTimezonePromptandaddDismissedPromptcleanly implement the intended rules: exact match skip, strict< expiryMswindow, and replacement of prior entries for the same(saved, detected)pair while preserving others. The API is straightforward and test‑friendly.apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.test.ts (1)
11-162: Prompt visibility tests cover key edge casesThe
shouldShowTimezonePromptsuite exercises matching TZs, mismatches without history, recent vs expired dismissals, multiple combinations, and the exact expiry boundary, which aligns well with the helper’s semantics. Nice use of fake timers/system time to make these deterministic.
There was a problem hiding this comment.
1 issue found across 3 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx">
<violation number="1" location="apps/web/app/(app)/[emailAccountId]/calendars/TimezoneDetector.tsx:58">
If the timezone update action fails, this effect keeps firing `executeUpdateTimezone` on every revalidation, creating an infinite loop of failed requests. Add a guard (e.g., track an attempted update/ref) before auto-invoking the action so failures don’t keep hammering the endpoint.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/web/utils/calendar/providers/microsoft-availability.ts (2)
26-68: N+1 query pattern causes performance bottleneck.The loop makes one API call per calendar, creating a performance bottleneck and increasing the risk of hitting API rate limits. Microsoft Graph API's JSON batching should be used to combine these requests into a single call.
39-61: Missing pagination handling causes incomplete busy period data.The code only processes the first page of results. Microsoft Graph API paginates calendarView responses, and subsequent pages (via
@odata.nextLink) are not followed. This means busy events beyond the default page size are dropped, leading to incorrect availability calculations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/utils/calendar/providers/microsoft-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/outlook/calendar-client.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/calendar/providers/microsoft-availability.ts (3)
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/outlook/calendar-client.ts (1)
getCalendarClientWithRefresh(44-135)
⏰ 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). (5)
- GitHub Check: Software Component Analysis Js
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/web/utils/calendar/providers/microsoft-availability.ts (4)
1-10: LGTM: Imports and logger setup are correct.The imports are appropriate and the scoped logger follows the project's logging patterns.
62-74: LGTM: Error handling strategy is appropriate.The per-calendar error handling allows partial success (continuing with other calendars if one fails), while the outer try-catch handles catastrophic failures. This resilience pattern is appropriate for availability checking.
41-45: LGTM: Event filtering logic is correct.The filtering correctly excludes "free" events and ensures both start and end times exist before processing, preventing null/undefined errors.
77-103: LGTM: Provider implementation follows the interface correctly.The provider correctly implements the CalendarAvailabilityProvider interface, properly handles token refresh via
getCalendarClientWithRefresh, and cleanly delegates to the fetcher function.
There was a problem hiding this comment.
1 issue found across 3 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/utils/calendar/providers/microsoft-availability.ts">
<violation number="1" location="apps/web/utils/calendar/providers/microsoft-availability.ts:52">
Convert Windows timezone names to IANA before constructing TZDate to avoid incorrect conversions; also ensure event datetime values are treated correctly (UTC vs local) to prevent systematic offset errors.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
1 issue found across 1 file (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/utils/calendar/providers/microsoft-availability.ts">
<violation number="1" location="apps/web/utils/calendar/providers/microsoft-availability.ts:44">
This change introduces a critical timezone bug by incorrectly assuming the `dateTime` string from the Microsoft Graph API is a complete ISO 8601 UTC string. The string lacks a timezone identifier and will be parsed as local time, breaking the unified availability feature which requires consistent UTC data from all calendar providers.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/web/utils/calendar/providers/microsoft.ts (2)
29-35: Add requiredscopeparameter to token exchange.The token exchange request is missing the
scopefield, which Microsoft Identity's v2/tokenendpoint requires for theauthorization_codegrant. Without it, the OAuth flow will fail with error AADSTS900144.Apply this diff to add the scope parameter:
body: new URLSearchParams({ client_id: env.MICROSOFT_CLIENT_ID, client_secret: env.MICROSOFT_CLIENT_SECRET, code, grant_type: "authorization_code", redirect_uri: `${env.NEXT_PUBLIC_BASE_URL}/api/outlook/calendar/callback`, + scope: "offline_access Calendars.ReadWrite Calendars.Read.Shared User.Read", }),Note: The scope should match the scopes requested during authorization and must include
offline_accessto receive a refresh token. Based on the relevant code snippet fromcalendar-client.ts, you should reference theCALENDAR_SCOPESconstant if it's exported, or ensure consistency with the scopes used during token refresh.
125-132: Distinguish auth errors from transient failures before disconnecting.The catch block unconditionally marks the calendar connection as disconnected on ANY sync error (lines 127-130), including transient network failures, temporary Graph API outages, or database issues. This forces users to re-authenticate even when the credentials are valid.
Consider checking the error type before updating
isConnected:} catch (error) { logger.error("Error syncing calendars", { error, connectionId }); - await prisma.calendarConnection.update({ - where: { id: connectionId }, - data: { isConnected: false }, - }); + + // Only disconnect for auth/consent errors, not transient failures + const isAuthError = + error instanceof Error && + (error.message.includes("invalid_grant") || + error.message.includes("consent_required") || + error.message.includes("interaction_required")); + + if (isAuthError) { + await prisma.calendarConnection.update({ + where: { id: connectionId }, + data: { isConnected: false }, + }); + } + throw error; }Alternatively, enhance
getCalendarClientWithRefreshto throw a typed error (e.g.,AuthenticationError) for credential failures, then checkerror instanceof AuthenticationErrorhere.
🧹 Nitpick comments (3)
apps/web/utils/calendar/providers/microsoft.ts (1)
96-124: Consider fetching calendar timezone information.The calendar sync logic correctly upserts calendars, but the
timezonefield is set tonull. Looking atfetchMicrosoftCalendarsincalendar-client.ts(lines 136-155), the query doesn't select thetimeZonefield. WhileautoPopulateTimezoneis called on line 124, it won't be able to extract timezone data from Microsoft calendars without this field.If timezone information is available from Microsoft Graph API, consider:
- Updating
fetchMicrosoftCalendarsto select thetimeZonefield in the API query- Passing that timezone value to the calendar upsert operations (lines 111, 118)
This would allow
autoPopulateTimezoneto properly populate the user's timezone from their Microsoft calendars.apps/web/app/(app)/[emailAccountId]/calendars/CalendarSettings.tsx (2)
26-55: Timezone options and auto-detect flow are solid (with a minor optional guard)The curated
BASE_TIMEZONESplus the injected auto-detect option and fallback to the stored timezone cover both common regions and any valid stored IANA zone, which solves the earlier “unlisted timezone” issue. The only minor edge is ifBASE_TIMEZONESwere ever edited to remove"UTC",utcIndexwould be-1andsplice(utcIndex + 1, …)would insert at the start; harmless but you could defensively handleutcIndex === -1if the list is likely to change.Also applies to: 63-85
128-139: Confirm intended behavior whentimezoneisnullThe timezone form only calls
resetTimezonewhentimezone !== null, so if the backend returnsnull(meaning “unset”), the form keeps its initial default"UTC"rather than reflecting a distinct “unset” state. Ifnullis intended to be interpreted as"UTC", this is fine; otherwise you may want to reset on any data load (similar to the booking link effect) and choose a different default.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/app/(app)/[emailAccountId]/calendars/CalendarSettings.tsx(1 hunks)apps/web/app/api/user/email-account/route.ts(1 hunks)apps/web/utils/actions/ai-rule.ts(3 hunks)apps/web/utils/actions/assess.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.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/user/validate.ts(1 hunks)apps/web/utils/webhook/validate-webhook-account.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 (2)
apps/web/utils/calendar/providers/microsoft.ts (5)
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-156)apps/web/utils/calendar/timezone-helpers.ts (1)
autoPopulateTimezone(7-39)
apps/web/app/(app)/[emailAccountId]/calendars/CalendarSettings.tsx (6)
apps/web/providers/EmailAccountProvider.tsx (1)
useAccount(79-89)apps/web/hooks/useCalendars.ts (1)
useCalendars(4-6)apps/web/utils/actions/calendar.ts (2)
updateEmailAccountTimezoneAction(63-71)updateCalendarBookingLinkAction(73-81)apps/web/utils/actions/calendar.validation.ts (2)
updateTimezoneBody(14-16)updateBookingLinkBody(19-25)apps/web/components/SettingCard.tsx (1)
SettingCard(3-34)apps/web/components/LoadingContent.tsx (1)
LoadingContent(13-27)
⏰ 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 (10)
apps/web/utils/calendar/providers/microsoft.ts (2)
1-14: LGTM!The imports, logger setup, and provider structure correctly implement the CalendarOAuthProvider interface.
39-79: LGTM!The profile fetching, email extraction, refresh token validation, and token response handling are all correctly implemented with appropriate error messages.
apps/web/utils/webhook/validate-webhook-account.ts (1)
19-20: LGTM!The addition of
timezoneandcalendarBookingLinkfields to the select projection is correct and aligns with the broader PR changes to support calendar settings.apps/web/utils/assistant/process-assistant-email.ts (1)
110-111: LGTM!The addition of
timezoneandcalendarBookingLinkfields to the EmailAccount query is consistent with the calendar integration features being added.apps/web/app/api/user/email-account/route.ts (1)
22-23: LGTM!Adding
timezoneandcalendarBookingLinkto the EmailAccount API response is appropriate for exposing calendar settings to clients.apps/web/utils/user/validate.ts (1)
18-19: LGTM!The addition of
timezoneandcalendarBookingLinkfields maintains consistency with the calendar integration features.apps/web/utils/actions/assess.ts (1)
50-51: Based on verification of all EmailAccount queries in the codebase, the field additions are comprehensive and follow proper context-aware Prisma patterns. The fieldstimezoneandcalendarBookingLinkappear in all user profile retrieval operations where they're semantically needed:
- User profile endpoints:
apps/web/utils/user/get.ts,apps/web/utils/user/validate.ts,apps/web/app/api/user/calendars/route.ts,apps/web/app/api/ai/analyze-sender-pattern/route.ts, and AI action files all correctly include both fields- Assistant/AI operations:
apps/web/utils/assistant/process-assistant-email.tsincludes them- Authorization-only queries: Correctly select only
id,- Webhook/subscription validation: Appropriately use minimal select for their narrow use cases
Selecting only the fields and relations that you require rather than relying on the default selection set reduces the size of the response and improves query speed. The codebase demonstrates this principle consistently—queries add these fields precisely where user profile operations need them, not universally.
LGTM! Coverage is complete.
apps/web/utils/actions/ai-rule.ts (2)
141-142: Dynamic test IDs look goodUsing
Date.now()in the test message/thread IDs avoids collisions across test runs and stays isolated from normal execution. No issues from a behavior or type perspective.
203-228: IncludingtimezoneandcalendarBookingLinkinemailAccountselects is appropriateSelecting
timezoneandcalendarBookingLinkhere keeps theemailAccountshape consistent with the broader model changes and gives downstream AI helpers access to calendar context without extra queries. This looks aligned and non-breaking for existing logic.Also applies to: 441-466
apps/web/app/(app)/[emailAccountId]/calendars/CalendarSettings.tsx (1)
86-103: Action wiring and form UX for timezone and booking link look goodUsing
useActionfor both updates, converting the"auto-detect"sentinel into the real IANA timezone before persisting, and callingmutate()+ success toasts gives a clean and predictable UX. The forms are simple, validation is delegated to zod, and the loading/error handling viaLoadingContent/Skeletonis consistent with the rest of the app.Also applies to: 141-162, 164-235
There was a problem hiding this comment.
1 issue found across 10 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/utils/calendar/providers/microsoft.ts">
<violation number="1" location="apps/web/utils/calendar/providers/microsoft.ts:111">
Setting the stored calendar timezone to null breaks the availability fallback that reads `calendar.timezone`, causing Microsoft calendars without an email-level timezone to default to UTC and produce incorrect availability.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| update: { | ||
| name: microsoftCalendar.name || "Untitled Calendar", | ||
| description: microsoftCalendar.description, | ||
| timezone: null, |
There was a problem hiding this comment.
Setting the stored calendar timezone to null breaks the availability fallback that reads calendar.timezone, causing Microsoft calendars without an email-level timezone to default to UTC and produce incorrect availability.
Prompt for AI agents
Address the following comment on apps/web/utils/calendar/providers/microsoft.ts at line 111:
<comment>Setting the stored calendar timezone to null breaks the availability fallback that reads `calendar.timezone`, causing Microsoft calendars without an email-level timezone to default to UTC and produce incorrect availability.</comment>
<file context>
@@ -108,14 +108,14 @@ export const microsoftCalendarProvider: CalendarOAuthProvider = {
name: microsoftCalendar.name || "Untitled Calendar",
description: microsoftCalendar.description,
- timezone: microsoftCalendar.timeZone,
+ timezone: null,
},
create: {
</file context>
| timezone: null, | |
| timezone: microsoftCalendar.timeZone, |
2 participants
Summary by CodeRabbit
New Features
UX
Tests
Chores