Conversation
…ous API routes. Refactor rate limiting checks and streamline response handling for unauthorized access.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis set of changes introduces improved error handling and authentication consistency across several API routes by wrapping handlers in try-catch blocks, logging errors, and forcefully logging out users upon errors. The logout process now also deletes the active connection before signing out. Rate limiting is standardized and added to more endpoints. The Google driver reduces its OAuth2 scope and removes retry logic for Gmail API calls. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler
participant Auth
participant DB
participant Utils
Client->>API_Handler: Send GET/POST/PUT request
API_Handler->>Auth: Authenticate user (getAuthenticatedUserId)
Auth-->>API_Handler: User ID / Error
API_Handler->>Utils: Apply rate limiting
Utils-->>API_Handler: Rate limit status
alt Rate limit exceeded
API_Handler-->>Client: 429 Too Many Requests
else Authenticated and allowed
API_Handler->>DB: Perform requested DB operation
DB-->>API_Handler: Data/result
API_Handler-->>Client: Return response
end
alt Error occurs
API_Handler->>Utils: logoutUser (delete connection, sign out)
Utils-->>API_Handler: Done
API_Handler-->>Client: Return empty/structured error response
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (2)
apps/mail/lib/hotkeys/use-hotkey-utils.ts (1)
133-145:⚠️ Potential issueDisabled functionality without explanation or alternative
The entire internal logic of the
useShortcutsfunction has been commented out, effectively disabling keyboard shortcut registration. This means any component calling this function will silently fail to register shortcuts.Consider one of the following approaches instead of commenting out code:
- Use a feature flag to conditionally enable/disable shortcuts
- Return early with a clear reason
- Add a proper explanation in code comments about why this is disabled and when it will be re-enabled
export function useShortcuts( shortcuts: Shortcut[], handlers: { [key: string]: () => void }, options: Partial<HotkeyOptions> = {}, ) { - // DISABLED - // shortcuts.forEach((shortcut) => { - // const handler = handlers[shortcut.action]; - // if (handler) { - // useShortcut(shortcut, handler, options); - // } - // }); + // If there's a reason to disable shortcuts temporarily, explain why + const enableShortcuts = process.env.ENABLE_SHORTCUTS !== 'false'; + if (!enableShortcuts) { + console.debug('Keyboard shortcuts are currently disabled'); + return; + } + + shortcuts.forEach((shortcut) => { + const handler = handlers[shortcut.action]; + if (handler) { + useShortcut(shortcut, handler, options); + } + }); }apps/mail/components/mail/mail-list.tsx (1)
497-514:⚠️ Potential issueMissing dependencies in the
handleMailClickuseCallbackThe dependency array for
handleMailClickis empty, but the function useshandleMouseEnter,setMail, andsetThreadId. This could cause stale closures where the callback captures outdated values.const handleMailClick = useCallback( (message: ParsedMessage) => () => { handleMouseEnter(message.id); const messageThreadId = message.threadId ?? message.id; // Update local state immediately for optimistic UI setMail((prev) => ({ ...prev, replyComposerOpen: false, forwardComposerOpen: false, })); // Update URL param without navigation void setThreadId(messageThreadId); }, - [], + [handleMouseEnter, setMail, setThreadId], );
🧹 Nitpick comments (4)
apps/mail/app/api/driver/connections/route.ts (1)
16-50: Good addition of robust error handling with graceful degradation.The try-catch block improves error handling by logging errors, logging out the user when authentication issues occur, and returning an empty array response instead of crashing or returning a 500 error.
This pattern ensures better user experience and security by preventing users from remaining in potentially invalid sessions when errors occur.
Consider implementing more granular error handling for certain types of errors that might not require logging the user out (e.g., transient database errors that could be retried).
apps/mail/app/api/driver/notes/route.ts (1)
14-44: Good addition of robust error handling with graceful degradation.The try-catch block improves error handling by logging errors, logging out the user when authentication issues occur, and returning an empty array response instead of crashing or returning a 500 error.
This pattern ensures better user experience and security by preventing users from remaining in potentially invalid sessions when errors occur.
Consider implementing more granular error handling for certain types of errors that might not require logging the user out (e.g., temporary database connection issues).
apps/mail/app/api/driver/count/route.ts (1)
13-37: Good addition of robust error handling with graceful degradation.The try-catch block improves error handling by logging errors, logging out the user when authentication issues occur, and returning an empty object response instead of crashing or returning a 500 error.
This pattern ensures better user experience and security by preventing users from remaining in potentially invalid sessions when errors occur.
Consider implementing more granular error handling for certain types of errors that might not require logging the user out (e.g., temporary service unavailability).
apps/mail/app/api/v1/hotkeys/route.ts (1)
1-9: Consider adding try-catch blocks for error handlingWhile the authentication and rate limiting improvements are good, consider adding try-catch blocks around database operations to align with the PR's objective of improving error handling across the app.
You could wrap the database operations in try-catch blocks and implement proper error handling:
export async function GET() { const userId = await getAuthenticatedUserId(); const ratelimit = getRatelimitModule({ prefix: 'ratelimit:hotkeys', limiter: Ratelimit.slidingWindow(60, '1m'), }); const { success, headers } = await checkRateLimit(ratelimit, userId); if (!success) { return NextResponse.json( { error: 'Too many requests. Please try again later.' }, { status: 429, headers }, ); } + try { const result = await db.select().from(userHotkeys).where(eq(userHotkeys.userId, userId)); return NextResponse.json(result[0]?.shortcuts || []); + } catch (error) { + console.error('Error fetching user hotkeys:', error); + return NextResponse.json( + { error: 'Failed to fetch hotkeys' }, + { status: 500 } + ); + } }Apply similar error handling to POST and PUT methods as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/mail/app/api/driver/connections/route.ts(2 hunks)apps/mail/app/api/driver/count/route.ts(1 hunks)apps/mail/app/api/driver/google.ts(2 hunks)apps/mail/app/api/driver/notes/route.ts(1 hunks)apps/mail/app/api/driver/route.ts(1 hunks)apps/mail/app/api/utils.ts(2 hunks)apps/mail/app/api/v1/hotkeys/route.ts(4 hunks)apps/mail/components/mail/mail-list.tsx(1 hunks)apps/mail/components/mail/mail-quick-actions.tsx(8 hunks)apps/mail/components/mail/thread-display.tsx(9 hunks)apps/mail/hooks/use-threads.ts(1 hunks)apps/mail/lib/hotkeys/mail-list-hotkeys.tsx(1 hunks)apps/mail/lib/hotkeys/use-hotkey-utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
apps/mail/app/api/utils.ts (1)
apps/mail/actions/utils.ts (1)
deleteActiveConnection(12-29)
apps/mail/app/api/driver/connections/route.ts (4)
apps/mail/app/api/utils.ts (5)
getAuthenticatedUserId(35-44)processIP(64-73)getRatelimitModule(9-21)checkRateLimit(53-62)logoutUser(47-51)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (1)
connection(77-94)apps/mail/types/index.ts (1)
IConnection(64-69)
apps/mail/app/api/driver/count/route.ts (7)
apps/mail/app/api/driver/connections/route.ts (1)
GET(15-51)apps/mail/app/api/driver/route.ts (1)
GET(13-50)apps/mail/app/api/v1/hotkeys/route.ts (1)
GET(11-30)apps/mail/app/api/driver/notes/route.ts (1)
GET(13-45)apps/mail/app/api/driver/[id]/route.ts (1)
GET(6-27)apps/mail/app/api/utils.ts (5)
getAuthenticatedUserId(35-44)processIP(64-73)getRatelimitModule(9-21)checkRateLimit(53-62)logoutUser(47-51)apps/mail/actions/utils.ts (1)
getActiveDriver(31-61)
apps/mail/app/api/driver/notes/route.ts (2)
apps/mail/app/api/utils.ts (5)
getAuthenticatedUserId(35-44)processIP(64-73)getRatelimitModule(9-21)checkRateLimit(53-62)logoutUser(47-51)apps/mail/app/api/notes/db.ts (1)
notesManager(6-157)
apps/mail/hooks/use-threads.ts (1)
apps/mail/lib/utils.ts (1)
defaultPageSize(143-143)
apps/mail/app/api/v1/hotkeys/route.ts (4)
apps/mail/app/api/utils.ts (3)
getAuthenticatedUserId(35-44)getRatelimitModule(9-21)checkRateLimit(53-62)packages/db/src/index.ts (1)
db(17-17)packages/db/src/schema.ts (1)
userHotkeys(50-57)apps/mail/config/shortcuts.ts (1)
Shortcut(3-10)
🔇 Additional comments (25)
apps/mail/app/api/utils.ts (1)
46-51: Good improvement to logout processThe logout function now properly cleans up by deleting the active connection before signing out the user. The added comment is helpful for clarifying the function's purpose.
apps/mail/lib/hotkeys/mail-list-hotkeys.tsx (1)
35-50: Simplified ID selection logicThe ID selection logic has been simplified to consistently use
item.idinstead of possibly falling back to another property likethreadId. This improves code consistency.However, note that this function is still being defined and used even though the
useShortcutsfunction inuse-hotkey-utils.tshas been disabled, meaning this shortcut won't actually work. Consider addressing this inconsistency.apps/mail/app/api/driver/route.ts (1)
13-50: Improved error handling with proper cleanupThe error handling has been improved by wrapping the code in a try-catch block, logging the error, properly logging out the user (which now includes deleting the active connection), and returning a valid JSON response instead of redirecting.
Consider adding a status code to the error response:
catch (error) { console.warn('Error getting threads:', error); await logoutUser(); - return NextResponse.json({ messages: [], nextPageToken: null }); + return NextResponse.json({ messages: [], nextPageToken: null }, { status: 401 }); }apps/mail/app/api/driver/connections/route.ts (1)
1-7: Import additions support enhanced error handling.The import of
logoutUserenables the new error handling strategy for this endpoint.apps/mail/app/api/driver/google.ts (1)
132-132: Security improvement: Reduced OAuth2 scope.Reducing the OAuth scope to only
gmail.modifyfollows the principle of least privilege by requesting only the permissions needed for the application to function, which is a security best practice.apps/mail/app/api/driver/notes/route.ts (1)
1-7: Import additions support enhanced error handling.The import of
logoutUserenables the new error handling strategy for this endpoint.apps/mail/app/api/driver/count/route.ts (1)
1-7: Import additions support enhanced error handling.The import of
logoutUserenables the new error handling strategy for this endpoint.apps/mail/hooks/use-threads.ts (1)
94-104: LGTM - Improved request parameter handlingThe refactoring of the fetcher function to dynamically construct query parameters inside the function is cleaner than using a static
searchParamsobject outside. This ensures that each request uses the latest parameter values.apps/mail/components/mail/mail-quick-actions.tsx (3)
35-37: LGTM - Improved data fetching with useThread hookUsing the
useThreadhook to fetch the latest thread data directly inside the component is better than relying solely on the passed message prop, ensuring you always work with up-to-date information.
50-51: Good defensive programming with null checksAdding null checks for
latestMessagebefore attempting to use it prevents potential runtime errors when thread data hasn't loaded yet.Also applies to: 65-66, 107-108
169-170: LGTM - Better read status checkingUsing
threadData?.hasUnreadfor the label is more reliable than checking the message's unread status directly, as it accounts for the entire thread's state.apps/mail/app/api/v1/hotkeys/route.ts (3)
11-30: LGTM - Added proper authentication and rate limiting to GET endpointThe implementation of centralized authentication and rate limiting provides better security and abuse prevention for the API.
32-67: LGTM - Added proper authentication and rate limiting to POST endpointConsistent application of the same authentication and rate limiting patterns to all endpoints is good practice.
69-115: LGTM - Added proper authentication and rate limiting to PUT endpointThe rate limiting with custom prefixes for each method type allows for more granular control over API usage.
apps/mail/components/mail/thread-display.tsx (11)
14-19: Appropriate addition of navigation and mail action imports
The new imports (useParams,moveThreadsTo,ThreadDestination,markAsRead,markAsUnread) are correctly added and used throughout the component for folder routing and thread/mail actions.
28-34: Import of NotesPanel and toast is utilized correctly
TheNotesPanelcomponent andtoastfromsonnerare now imported to support note display and UI notifications. They are properly integrated in the action toolbar and error–notification flows.
87-87: Stylistic update inThreadDemoreply composer
TheReplyComposecall inThreadDemonow consistently uses arrow‐function prop formatting. Behavior remains unchanged.
149-157:hasImagesuseMemologic is clear and correct
The updated image‐detection logic insideuseMemocorrectly checks attachments and inline<img>tags (excluding SVG avatars). Consider extracting this predicate into a named helper for readability, but the implementation is sound.
159-162:hasMultipleParticipantscalculation is concise
The logic for detecting multiple participants correctly sums recipients and sender, driving the “Reply All” UI.
168-176: Verify.thenafter.catchinmarkAsRead
The chainmarkAsRead(...).catch(...).then(...)will invokemutateThread()andmutateStats()even if marking as read fails. Confirm whether the intent is to always refresh, or if you’d prefer separating success and error flows (e.g., using.finally(...)).
193-194:handleCloseresetsthreadIdcorrectly
Clearing thethreadIdquery state on close ensures proper navigation and unmounting of the thread view.
228-235:handleMarkAsUnreadflow is robust
ThemarkAsUnreadhandler leveragestoast.promisefor loading/success/error feedback and correctly resets bulk selection, stats, and thread data on completion.
376-383: Composer toggle UI enhancements are consistent
The class toggles andonClickhandlers for Reply, Reply All, and Forward buttons now uniformly manage theAlso applies to: 391-398, 406-413
421-429: Image‐hidden warning panel is user-friendly
Displaying a warning banner when images are hidden, with a clear CTA to show them, provides a good user experience and ensures inline images are opt‑in.
454-464: BottomReplyComposeplacement and mode logic
The bottom-mountedReplyComposenow respects the current mode (forward,replyAll, orreply) and adjusts for fullscreen vs. embedded views, mirroring the top toolbar logic.
| const res = await gmail.users.threads.list({ | ||
| userId: 'me', | ||
| q: normalizedQ ? normalizedQ : undefined, | ||
| labelIds: folder === 'inbox' ? labelIds : [], | ||
| maxResults, | ||
| pageToken: pageToken ? pageToken : undefined, | ||
| quotaUser: config.auth?.email, | ||
| }); | ||
| return { ...res.data, threads: res.data.threads } as any; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removed retry logic for Gmail API calls.
The removal of the withExponentialBackoff wrapper means the Gmail API call no longer has built-in retry logic for rate limiting issues.
This change could reduce the resilience to transient rate limit errors. Ensure that rate limit handling is implemented elsewhere, or consider restoring the retry logic:
🏁 Script executed:
#!/bin/bash
# Check for rate limiting error handling elsewhere
echo "Searching for rate limit error handling:"
rg -A 5 -B 5 "rate.*limit|429" --type ts apps/mail/ | grep -v "gmail-v1"Length of output: 25720
🏁 Script executed:
#!/bin/bash
# Display lines around retry logic and the list call in google.ts
sed -n '480,600p' apps/mail/app/api/driver/google.tsLength of output: 4553
Add retry with exponential backoff to the list call
The list method in apps/mail/app/api/driver/google.ts now invokes gmail.users.threads.list directly, whereas the get method still uses your withExponentialBackoff wrapper. Without backoff retries, transient 429/rate‑limit errors will surface immediately, reducing resilience.
• File: apps/mail/app/api/driver/google.ts
• Around lines ~528–536
Suggested change:
list: async (
folder: string,
q: string,
maxResults = 20,
_labelIds: string[] = [],
pageToken?: string,
) => {
const { folder: normalizedFolder, q: normalizedQ } = normalizeSearch(folder, q ?? '');
const labelIds = [..._labelIds];
if (normalizedFolder) labelIds.push(normalizedFolder.toUpperCase());
- const res = await gmail.users.threads.list({
- userId: 'me',
- q: normalizedQ || undefined,
- labelIds: folder === 'inbox' ? labelIds : [],
- maxResults,
- pageToken: pageToken || undefined,
- quotaUser: config.auth?.email,
- });
- return { ...res.data, threads: res.data.threads } as any;
+ return withExponentialBackoff(async () => {
+ const res = await gmail.users.threads.list({
+ userId: 'me',
+ q: normalizedQ || undefined,
+ labelIds: folder === 'inbox' ? labelIds : [],
+ maxResults,
+ pageToken: pageToken || undefined,
+ quotaUser: config.auth?.email,
+ });
+ return { ...res.data, threads: res.data.threads } as any;
+ });
},This restores automatic retries on rate‑limit and other transient errors, matching your existing get implementation.
📝 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.
| const res = await gmail.users.threads.list({ | |
| userId: 'me', | |
| q: normalizedQ ? normalizedQ : undefined, | |
| labelIds: folder === 'inbox' ? labelIds : [], | |
| maxResults, | |
| pageToken: pageToken ? pageToken : undefined, | |
| quotaUser: config.auth?.email, | |
| }); | |
| return { ...res.data, threads: res.data.threads } as any; | |
| list: async ( | |
| folder: string, | |
| q: string, | |
| maxResults = 20, | |
| _labelIds: string[] = [], | |
| pageToken?: string, | |
| ) => { | |
| const { folder: normalizedFolder, q: normalizedQ } = normalizeSearch(folder, q ?? ''); | |
| const labelIds = [..._labelIds]; | |
| if (normalizedFolder) labelIds.push(normalizedFolder.toUpperCase()); | |
| return withExponentialBackoff(async () => { | |
| const res = await gmail.users.threads.list({ | |
| userId: 'me', | |
| q: normalizedQ || undefined, | |
| labelIds: folder === 'inbox' ? labelIds : [], | |
| maxResults, | |
| pageToken: pageToken || undefined, | |
| quotaUser: config.auth?.email, | |
| }); | |
| return { ...res.data, threads: res.data.threads } as any; | |
| }); | |
| }, |
| mutateThread(); | ||
| toast.success(t('common.mail.markedAsRead')); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify mutateThread usage in dependency array
You're now using mutateThread from useThread instead of mutate from useThreads, but mutate is still listed in the dependency array at line 140.
},
- [latestMessage, mutate, t, isProcessing, isLoading, closeThreadIfOpen],
+ [latestMessage, mutateThread, t, isProcessing, isLoading, closeThreadIfOpen],Let's check all dependencies in the handleToggleRead function:
Also applies to: 126-127
🏁 Script executed:
#!/bin/bash
# Check where mutate from useThreads is used in handleToggleRead
grep -n "mutate" -A 10 -B 10 apps/mail/components/mail/mail-quick-actions.tsx | grep -A 20 -B 20 "handleToggleRead"Length of output: 1467
🏁 Script executed:
#!/bin/bash
# Display the handleToggleRead useCallback and its dependency array
grep -n "const handleToggleRead" -A 20 -B 5 apps/mail/components/mail/mail-quick-actions.tsxLength of output: 1044
🏁 Script executed:
#!/bin/bash
# Extract handleToggleRead callback and its dependencies
sed -n '104,160p' apps/mail/components/mail/mail-quick-actions.tsxLength of output: 1817
Update handleToggleRead’s dependencies to use mutateThread
In apps/mail/components/mail/mail-quick-actions.tsx, the handleToggleRead callback calls mutateThread() but its dependency array still lists the old mutate. This can lead to stale closures and missed updates.
• Location: inside the useCallback for handleToggleRead (around line 104–112)
• Change the dependency array as follows:
[
- latestMessage, mutate, t, isProcessing, isLoading, closeThreadIfOpen
+ latestMessage, mutateThread, t, isProcessing, isLoading, closeThreadIfOpen
],1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Style
Chores