Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughError handling for Microsoft Outlook authentication has been refined. The Outlook client now detects specific Azure AD error codes and throws user-friendly Changes
Sequence DiagramsequenceDiagram
participant Client as Client Request
participant OutlookClient as Outlook Client
participant Middleware as Email Middleware
participant ErrorHandler as Error Handler
Client->>OutlookClient: Request (with expired token)
OutlookClient->>OutlookClient: Detect AADSTS error code
OutlookClient->>OutlookClient: Log warning
OutlookClient->>Middleware: Throw SafeError
rect rgb(220, 240, 255)
Note over Middleware: NEW: Check if SafeError
Middleware->>Middleware: Identify as SafeError
Middleware->>ErrorHandler: Re-throw SafeError
end
ErrorHandler->>Client: User-friendly re-auth message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/outlook/client.ts (1)
146-178: Re‑auth SafeError flow is sound; consider tightening overlap with catch‑block loggingThe new
requiresReauthdetection for specific AADSTS /invalid_grantcases looks good and, combined with throwing aSafeError, gives you a clean, user‑facing “sign in again” message that will propagate throughemailProviderMiddlewareandwithMiddlewareas intended.One minor cleanup you could consider: since these same
invalid_grant/AADSTS50173cases now throw aSafeErrorwith a human‑readable message, theisInvalidGrantErrorcheck in thecatchblock (which looks for those substrings onerror.message) will no longer match for this path. Logging is already covered by the newlogger.warn("Microsoft authorization expired - user needs to reconnect", ...), so you might either:
- Narrow
isInvalidGrantErrorto only the remaining scenarios you still expect there, or- Remove the overlapping substrings from the catch‑block check to avoid confusion about when that logging actually triggers.
Purely optional, since behavior is already correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/middleware.ts(1 hunks)apps/web/utils/outlook/client.ts(1 hunks)
🧰 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/middleware.ts
🧬 Code graph analysis (2)
apps/web/utils/middleware.ts (1)
apps/web/utils/error.ts (1)
SafeError(59-69)
apps/web/utils/outlook/client.ts (1)
apps/web/utils/error.ts (1)
SafeError(59-69)
⏰ 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: cubic · AI code reviewer
- GitHub Check: Review for correctness
- GitHub Check: test
🔇 Additional comments (1)
apps/web/utils/middleware.ts (1)
302-317: SafeError rethrow correctly preserves user‑friendly auth errorsRe‑throwing
SafeErrorfromemailProviderMiddlewarelets the top‑levelwithMiddlewarehandler apply its dedicatedSafeErrorlogic (including the special “No refresh token” path and generic safe‑message 400s) instead of masking these as a 500 “Failed to initialize email provider”. This is a clean way to surface the new Outlook re‑auth messaging without affecting other error paths.
Show Outlook sign-in again by throwing
SafeErrorinapps/web/utils/outlook/client.ts:getOutlookClientWithRefreshwhen Microsoft identity errors (e.g., AADSTS70000, AADSTS70008, AADSTS70011, AADSTS700082, AADSTS50173, AADSTS65001, AADSTS500011, AADSTS54005, invalid_grant) require re-authenticationPropagate
SafeErrorfromemailProviderMiddlewareinstead of returning 500, and emit aSafeErrorwith a reconnect message fromgetOutlookClientWithRefreshon specific Microsoft identity errors.📍Where to Start
Start with the error handling branch in
getOutlookClientWithRefreshin client.ts, then review theSafeErrorguard in middleware.ts.📊 Macroscope summarized 977a164. 2 files reviewed, 4 issues evaluated, 4 issues filtered, 0 comments posted
🗂️ Filtered Issues
apps/web/utils/middleware.ts — 0 comments posted, 1 evaluated, 1 filtered
emailAccount.accountwhen readingemailAccount.account.provider. The Prisma query includesaccountwith onlyproviderselected, but the relation may benull(e.g., missing linked account). In that caseemailAccount.account.providerwill throw at runtime with "Cannot read properties of null (reading 'provider')" beforecreateEmailProvideris called, leading to a 500 response from the catch block. Add a guard to verifyemailAccount.accountandemailAccount.account.providerexist, and return a defined error (e.g., 404/400) rather than crashing. [ Out of scope ]apps/web/utils/outlook/client.ts — 0 comments posted, 3 evaluated, 3 filtered
expiresAtis compared againstDate.now()(milliseconds) on line 100, but the function later persistsexpires_atas seconds viaMath.floor(Date.now() / 1000 + tokens.expires_in)(line 187). On subsequent calls, a seconds-basedexpiresAtwill be compared to milliseconds, causing premature refresh attempts (or always-refresh). Fix by normalizing units (either store/read milliseconds or divideDate.now()by 1000). [ Out of scope ]refresh_tokenon refresh. The code persists onlyaccess_tokenandexpires_at(lines 184–192) and continues using the oldrefreshToken. This can lead to future refresh failures once the old refresh token is invalidated. Capture and persisttokens.refresh_tokenwhen present. [ Low confidence ]tokens.access_token(line 194) andtokens.expires_in(line 187) without checking presence or type. If either is missing or malformed,Math.floor(Date.now() / 1000 + tokens.expires_in)yieldsNaNand persists an invalidexpires_at, andcreateOutlookClient(tokens.access_token)will throw due to an undefined token after already saving broken state. Add explicit checks foraccess_tokenandexpires_inbefore saving/returning, and fail atomically. [ Low confidence ]Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.