Conversation
- added cc and bcc when saving drafts - save drafts less aggresively
chore: simplify and fix the dev env
* Create prompts with XML formatting * Include XML formatted prompts in generate func * remove unused regex and add helper functions/warnings * error handling * Update apps/mail/lib/prompts.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * lint issues * Update prompts.ts * #706 (comment) Coderabbit fix 1 * erabbitai bot 3 days ago⚠️ Potential issue errorOccurred state is stale inside finally React state setters (setErrorOccurred) are asynchronous; the errorOccurred value captured at render time will not yet reflect changes made earlier in the same event loop. Consequently, the logic deciding whether to collapse/expand may run with an outdated flag. - } finally { - setIsLoading(false); - if (!errorOccurred || isAskingQuestion) { - setIsExpanded(true); - } else { - setIsExpanded(false); // Collapse on errors - } - } + } finally { + setIsLoading(false); + // Use a local flag to track errors deterministically + const hadError = isAskingQuestion ? false : !!errorFlagRef.current; + setIsExpanded(!hadError); + } You can create const errorFlagRef = useRef(false); and update errorFlagRef.current = true every time an error is detected, ensuring reliable behaviour irrespective of React batching. Committable suggestion skipped: line range outside the PR's diff. * #706 (comment) * #706 (comment) * #706 (comment) --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…users (#726) * feat(i18n): add Vietnamese language support Add Vietnamese ('vi') to the list of supported languages in the i18n configuration and JSON file to expand language options. * Add a new Vietnamese translation file to support Vietnamese language users. * Clear Vietnamese translation strings
Co-authored-by: needle <122770437+needleXO@users.noreply.github.com>
* Updated lockfile * Updated home page session validation --------- Co-authored-by: Adam <x_1337@outlook.com>
* Create route og image * resolve coderabbit nitpicks --------- Co-authored-by: Adam <x_1337@outlook.com>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/mail/app/api/driver/connections/route.ts(2 hunks)apps/mail/app/api/driver/count/route.ts(2 hunks)apps/mail/app/api/driver/notes/route.ts(2 hunks)apps/mail/app/api/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/mail/app/api/driver/connections/route.ts
- apps/mail/app/api/driver/notes/route.ts
- apps/mail/app/api/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/mail/app/api/driver/count/route.ts (1)
apps/mail/app/api/utils.ts (1)
throwUnauthorizedGracefully(23-33)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/mail/actions/mail.ts (1)
33-33: Add proper await handling across all error handlersThe pattern of not properly awaiting
throwUnauthorizedGracefully()and immediately throwing an error after it is repeated in multiple catch blocks. This could lead to inconsistent error handling behavior.Apply the same fix to all similar occurrences:
- if (FatalErrors.includes((error as Error).message)) await throwUnauthorizedGracefully(); + if (FatalErrors.includes((error as Error).message)) { + await throwUnauthorizedGracefully(); + return; // Prevent execution continuing to throw error + }Also applies to: 45-45, 80-80, 122-122
🧹 Nitpick comments (2)
apps/mail/hooks/use-email-aliases.ts (2)
7-11: Type definition looks good but consider documentationThe
EmailAliastype definition is clear and well-structured. Moving it from an external import to an inline definition makes sense if this is the primary place it's used.Consider adding JSDoc comments to describe the purpose of this type and what each property represents, especially for optional fields like
nameandprimary.+/** + * Represents an email alias for a user + * @property email - The email address of the alias + * @property name - Optional display name for the alias + * @property primary - Whether this is the user's primary email alias + */ export type EmailAlias = { email: string; name?: string; primary?: boolean; };
13-25: Consider adding error handling documentationThe
useEmailAliaseshook implementation is clean and follows good practices. Consider adding documentation about what types of errors might be returned and how consumers should handle them.+/** + * Hook to fetch email aliases for the current user + * @returns {Object} Object containing aliases array, loading state, error state and mutate function + * @example + * const { aliases, isLoading, error } = useEmailAliases(); + * if (isLoading) return <Loader />; + * if (error) return <ErrorMessage message="Failed to load email aliases" />; + * return aliases.map(alias => <EmailCard key={alias.email} alias={alias} />); + */ export function useEmailAliases() { const { data, error, isLoading, mutate } = useSWRImmutable<EmailAlias[]>( '/api/v1/email-aliases', fetcher, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/mail/actions/email-aliases.ts(0 hunks)apps/mail/actions/mail.ts(6 hunks)apps/mail/actions/utils.ts(3 hunks)apps/mail/app/api/driver/connections/route.ts(2 hunks)apps/mail/app/api/driver/count/route.ts(2 hunks)apps/mail/app/api/driver/route.ts(2 hunks)apps/mail/app/api/utils.ts(2 hunks)apps/mail/app/api/v1/email-aliases/route.ts(1 hunks)apps/mail/hooks/use-email-aliases.ts(1 hunks)apps/mail/hooks/use-stats.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/mail/hooks/use-stats.ts
- apps/mail/actions/email-aliases.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/mail/app/api/driver/connections/route.ts
- apps/mail/actions/utils.ts
- apps/mail/app/api/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/mail/app/api/driver/route.ts (1)
apps/mail/app/api/utils.ts (1)
throwUnauthorizedGracefully(23-32)
apps/mail/actions/mail.ts (2)
apps/mail/actions/utils.ts (1)
FatalErrors(10-10)apps/mail/app/api/utils.ts (1)
throwUnauthorizedGracefully(23-32)
apps/mail/app/api/driver/count/route.ts (1)
apps/mail/app/api/utils.ts (1)
throwUnauthorizedGracefully(23-32)
🔇 Additional comments (2)
apps/mail/actions/mail.ts (1)
21-21: 🛠️ Refactor suggestionAdd
awaitkeyword for proper error handlingThe
throwUnauthorizedGracefully()function is async, but you're not properly awaiting it. This could lead to the error being thrown before the unauthorized handling completes.- if (FatalErrors.includes((error as Error).message)) await throwUnauthorizedGracefully(); + if (FatalErrors.includes((error as Error).message)) { + await throwUnauthorizedGracefully(); + return; // Prevent execution continuing to throw error + }Likely an incorrect or invalid review comment.
apps/mail/hooks/use-email-aliases.ts (1)
15-15: API endpoint update looks correctThe change from the previous endpoint to
/api/v1/email-aliasesaligns with the architectural changes mentioned in the summary, moving from direct server actions to a RESTful API with versioning.
…ror messages instead of throwing unauthorized errors. This improves readability and maintains functionality across various actions.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/mail/app/api/driver/[id]/route.ts (1)
29-34: Consider using a more descriptive error responseWhen an error occurs, the API now returns a 400 status with an empty thread object. While this prevents exposing internal error details, it might be helpful to include a generic error message to indicate that something went wrong, rather than just returning an empty result.
return NextResponse.json( - { messages: [], hasUnread: false, totalReplies: 0 }, + { messages: [], hasUnread: false, totalReplies: 0, error: 'Failed to retrieve thread' }, { status: 400, }, );apps/mail/actions/mail.ts (2)
42-74: Verbose logging in modifyLabels functionThe function contains multiple
console.logstatements that are likely for debugging purposes. Consider removing these or replacing them with a more structured logging approach in a production environment.- console.log(`Server: updateThreadLabels called for thread ${threadId}`); - console.log(`Adding labels: ${addLabels.join(', ')}`); - console.log(`Removing labels: ${removeLabels.join(', ')}`); try { const driver = await getActiveDriver(); const { threadIds } = driver.normalizeIds(threadId); if (threadIds.length) { await driver.modifyLabels(threadIds, { addLabels, removeLabels, }); - console.log('Server: Successfully updated thread labels'); return { success: true }; } - console.log('Server: No label changes specified'); return { success: false, error: 'No label changes specified' }; } catch (error) { console.error('Error updating thread labels:', error); throw error; }
76-115: Complex toggleStar implementation with potential for optimizationThe toggleStar function fetches each thread to determine if any message in the thread is already starred, which could be inefficient for large numbers of threads. Consider optimizing this by:
- Using a batch operation if the driver supports it
- Implementing a more efficient check for starred status
However, this appears to be existing code rather than a new implementation, so it may be outside the scope of the current PR.
apps/mail/app/api/driver/google.ts (1)
148-173:withSyncErrorHandlerduplicates logic – consolidate with the async variantBoth handlers build identical logging / standardisation code. You can DRY this up by internally delegating to
withErrorHandler, usingPromise.resolve(fn()), and then unwrapping the promise when necessary. This eliminates two maintenance paths and ensures behaviour stays consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/mail/actions/ai-reply.ts(1 hunks)apps/mail/actions/brain.ts(1 hunks)apps/mail/actions/connections.ts(5 hunks)apps/mail/actions/drafts.ts(1 hunks)apps/mail/actions/getSummary.ts(2 hunks)apps/mail/actions/mail.ts(1 hunks)apps/mail/actions/send.ts(1 hunks)apps/mail/actions/utils.ts(5 hunks)apps/mail/app/api/driver/[id]/route.ts(1 hunks)apps/mail/app/api/driver/connections/route.ts(3 hunks)apps/mail/app/api/driver/count/route.ts(2 hunks)apps/mail/app/api/driver/google.ts(5 hunks)apps/mail/app/api/driver/notes/route.ts(3 hunks)apps/mail/app/api/driver/route.ts(2 hunks)apps/mail/app/api/driver/types.ts(1 hunks)apps/mail/app/api/notes/index.ts(6 hunks)apps/mail/app/api/utils.ts(2 hunks)apps/mail/components/mail/thread-display.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/mail/actions/brain.ts
- apps/mail/actions/send.ts
- apps/mail/actions/getSummary.ts
- apps/mail/actions/ai-reply.ts
- apps/mail/app/api/driver/count/route.ts
- apps/mail/app/api/driver/connections/route.ts
- apps/mail/app/api/driver/route.ts
- apps/mail/actions/drafts.ts
- apps/mail/actions/utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/mail/app/api/utils.ts (2)
packages/db/src/schema.ts (1)
session(19-30)apps/mail/lib/auth.ts (1)
auth(299-302)
apps/mail/actions/mail.ts (1)
apps/mail/app/api/driver/types.ts (1)
IGetThreadResponse(3-8)
apps/mail/actions/connections.ts (2)
packages/db/src/schema.ts (1)
session(19-30)apps/mail/app/api/utils.ts (1)
getAuthenticatedUserId(24-29)
🔇 Additional comments (25)
apps/mail/app/api/driver/types.ts (1)
24-24: Improved type safety for thecountmethodThe return type of the
countmethod has been refined from a genericPromise<any>to a more specificPromise<{ count?: number; label?: string }[]>, which clearly defines the expected structure of the count data.This change improves type safety and provides better documentation for API consumers, making it clearer what to expect when calling this method.
apps/mail/app/api/driver/notes/route.ts (3)
1-1: Updated imports to remove logoutUser dependencyThe import statement has been updated to remove the
logoutUserdependency, which aligns with the overall refactoring to simplify error handling across the application.
27-27: Improved auth handling with explicit 401 responseAdded an early return with a 401 status code when the userId is not available, rather than proceeding and potentially causing errors later. This is a more RESTful approach to handling unauthorized requests.
36-38: Simplified error handling in catch blockThe error handling has been simplified to return a 400 status code with an empty array instead of calling
logoutUser(). This approach avoids forcing the user to log out on API errors, providing a better user experience.apps/mail/components/mail/thread-display.tsx (1)
18-23: Consolidated dropdown menu importsThe dropdown menu imports have been consolidated into a single import statement, improving code organization and readability.
apps/mail/actions/connections.ts (6)
3-3: Updated import to use getAuthenticatedUserIdThe import has been updated to use
getAuthenticatedUserIddirectly, removing the dependency onthrowUnauthorizedGracefully. This is part of the broader refactoring to standardize error handling.
16-17: Improved error handling with explicit error objectInstead of throwing an exception when the session is not found, the function now returns an explicit error object with a descriptive message. This provides a more consistent and predictable way to handle authentication errors.
22-23: Improved error handling with explicit error objectSimilar to the session check, the function now returns an explicit error object with a descriptive message when the user ID is not found, rather than throwing an exception.
44-45: Simplified authentication and error handlingThe session retrieval and user ID extraction have been consolidated into a single call to
getAuthenticatedUserId(), with an immediate return of an error object if the user ID is not available. This simplifies the code and makes it more readable.
54-55: Improved error handling with explicit error objectInstead of throwing an exception when the connection is not found, the function now returns an explicit error object with a descriptive message, consistent with the other error handling changes in this file.
67-67: Consistent error handling in catch blockThe catch block now returns an error object with the error message string instead of throwing, providing a consistent approach to error handling throughout the function. This matches the pattern used elsewhere in the function.
apps/mail/app/api/driver/[id]/route.ts (1)
7-28: Improved error handling implementationThe code has been refactored to wrap the entire GET handler in a try-catch block, which is a good practice for ensuring consistent error handling. This approach prevents unhandled exceptions from propagating up the call stack.
apps/mail/actions/mail.ts (5)
3-3: Simplified imports aligning with error handling standardizationThe import statement has been cleaned up by removing
deleteActiveConnection,FatalErrors, andthrowUnauthorizedGracefully, which aligns with the PR's goal of standardizing error handling across the codebase.
6-6: Return type updated to handle authentication failures gracefullyThe return type of
getMailnow includesnullas a possible return value, allowing the function to signal authentication failures without throwing exceptions, which is consistent with the PR's approach to error handling.
20-29: Consistent error handling in markAsReadThe error handling in this function follows the same pattern as the other functions, maintaining consistency in the codebase.
31-40: Consistent error handling in markAsUnreadThe error handling in this function follows the same pattern as the other functions, maintaining consistency in the codebase.
10-17:⚠️ Potential issueSimplified error handling in getMail
The error handling has been simplified to log and rethrow errors, which aligns with the standardized approach across the codebase. However, the function now returns
nullon error rather than throwing, which seems inconsistent with the implementation that still throws the error.try { const driver = await getActiveDriver(); const mailData = await driver.get(id); return mailData; } catch (error) { console.error('Error getting mail:', error); - throw error; + return null; }Likely an incorrect or invalid review comment.
apps/mail/app/api/utils.ts (2)
2-2: Consolidated imports from next/serverImports from next/server have been consolidated into a single line, which improves code readability.
24-29: Updated getAuthenticatedUserId to return null instead of throwingThe
getAuthenticatedUserIdfunction now returnsnullwhen no session exists, instead of throwing an exception. This aligns with the PR's goal of standardizing error handling.apps/mail/app/api/notes/index.ts (6)
7-16: Updated getCurrentUserId to return null instead of throwingThe
getCurrentUserIdfunction now returnsnullwhen no session exists, which aligns with the standardized error handling approach across the codebase.
19-28: Added null check for user ID in getThreadNotesThe function now includes a check for user ID and returns an empty array if it's falsy, which prevents errors when authentication fails and provides a clean fallback.
30-44: Added null check for user ID in createNoteThe function now includes a check for user ID and returns
nullif it's falsy, which prevents errors when authentication fails and provides a clean fallback.
46-58: Added null check for user ID in updateNoteThe function now includes a check for user ID and returns
nullif it's falsy, which prevents errors when authentication fails and provides a clean fallback.
60-69: Added null check for user ID in deleteNoteThe function now includes a check for user ID and returns
falseif it's falsy, which prevents errors when authentication fails and provides a clean fallback.
71-82: Added null check for user ID in reorderNotesThe function now includes a check for user ID and returns
falseif it's falsy, which prevents errors when authentication fails and provides a clean fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/mail/app/api/driver/google.ts (1)
917-929:⚠️ Potential issueFileReader is not available in a Node.js runtime
createDraftrelies on the browser‑onlyFileReaderAPI to convert attachments to Base‑64.
Because the Gmail driver runs on the server, this will throwReferenceError: FileReader is not definedin production.Use
Buffermethods instead (as already correctly done inparseOutgoing):- const base64Data = await new Promise<string>((resolve, reject) => { - const reader = new FileReader(); - reader.onload = () => { - const base64 = (reader.result as string).split(',')[1]; - if (base64) { - resolve(base64); - } else { - reject(new Error('Failed to read file as base64')); - } - }; - reader.onerror = () => reject(new Error('Failed to read file')); - reader.readAsDataURL(attachment); - }); + const arrayBuffer = await attachment.arrayBuffer(); + const base64Data = Buffer.from(arrayBuffer).toString('base64');
🧹 Nitpick comments (3)
apps/mail/app/api/driver/google.ts (3)
694-694: Avoid empty catch blocksThe empty catch block silently swallows errors during inline image processing. While non-critical, this could make debugging issues with inline images difficult.
- } catch (error) {} + } catch (error) { + // Non-critical: Continue without inline image if it fails to load + console.warn(`Failed to load inline image for message ${message.id}:`, error); + }
723-742: Improve error handling for attachment failuresThe code returns null on attachment fetch errors, which is better than swallowing errors entirely, but still doesn't provide visibility into what went wrong.
return null; } try { if (!message.id) { return null; } const attachmentData = await manager.getAttachment( message.id, attachmentId, ); return { filename: part.filename || '', mimeType: part.mimeType || '', size: Number(part.body?.size || 0), attachmentId: attachmentId, headers: part.headers || [], body: attachmentData ?? '', }; } catch (error) { + console.warn(`Failed to load attachment ${part.filename} for message ${message.id}:`, error); return null; }
472-474: Consider logging failure details in modifyThreadLabelsYou're collecting failure reasons but not logging or handling them. This lost information could be useful for debugging.
const failures = allResults.filter((result) => result.status === 'rejected'); if (failures.length > 0) { const failureReasons = failures.map((f) => ({ threadId: f.threadId, reason: f.reason })); + console.warn(`Failed to modify labels for ${failures.length} threads:`, failureReasons); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/app/api/driver/google.ts(5 hunks)
🔇 Additional comments (5)
apps/mail/app/api/driver/google.ts (5)
10-23: Good implementation of StandardizedError classThis is a proper implementation that addresses past feedback about returning Error subclasses instead of plain objects. The class now properly extends Error, preserves stack traces, and includes additional helpful context for debugging.
132-151: Nice centralized async error handling wrapperThe
withErrorHandlerimplementation is a great improvement that centralizes error handling across the codebase. The function appropriately:
- Logs errors with context
- Checks for fatal errors
- Cleans up resources by deleting active connections when needed
- Throws standardized errors for consistent downstream handling
This pattern will make debugging and error recovery much more consistent.
153-172: Good synchronous error handler implementationThe synchronous version complements the async version, following the same pattern but handling synchronous operations correctly with proper void promise handling for the connection deletion.
174-180: Good extraction of getScope functionMoving this function outside the driver factory improves reusability and makes the scopes more visible.
639-763: Good use of withExponentialBackoff for rate limitingWrapping the API call with
withExponentialBackoffis a great approach for handling rate limits, especially for thegetmethod which is likely to be called frequently.
9 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Style
Chores