Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@elie222 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces several significant changes across the application, focusing on email management, rule processing, and assistant functionality. The changes span multiple modules, including email parsing, rule creation, categorization, and AI-driven interactions. Key modifications include the removal of the onboarding interface, enhancements to email content extraction, improvements in rule management functions, the addition of new utility functions for group and sender handling, and the implementation of more robust email processing mechanisms. Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 4
🧹 Nitpick comments (5)
apps/web/utils/ai/assistant/process-user-request.ts (1)
37-44: Consider externalizing the system prompt for maintainabilityThe
systemprompt is currently defined inline within the function. To enhance maintainability and facilitate updates, consider externalizing the system prompt into a separate constant or configuration file.apps/web/utils/assistant/is-assistant-email.ts (1)
11-12: Normalize email addresses before comparison to handle case sensitivityEmail addresses are generally case-insensitive, especially in the local part. Comparing
assistantEmailandrecipientEmailwithout normalizing may lead to false negatives if the casing differs. Consider converting both emails to lowercase before comparison.Apply this diff to normalize the email addresses:
const assistantEmail = `${localPart}+${ASSISTANT_SUFFIX}@${domain}`; - return assistantEmail === recipientEmail; + return assistantEmail.toLowerCase() === recipientEmail.toLowerCase();apps/web/utils/assistant/is-assistant-email.test.ts (1)
21-27: Consider adding more test cases for edge scenarios.While the current test cases cover the basic scenarios, consider adding tests for:
- Case sensitivity (e.g.,
User+Assistant@example.com)- Special characters in email
- Null/undefined inputs
apps/web/utils/assistant/process-assistant-email.ts (1)
32-45: Consider adding retry logic for message retrieval.The message retrieval could fail due to transient issues. Consider implementing retry logic with exponential backoff.
+import { retry } from "@/utils/retry"; - const originalMessage = await getMessageByRfc822Id(originalMessageId, gmail); + const originalMessage = await retry( + () => getMessageByRfc822Id(originalMessageId, gmail), + { maxAttempts: 3, backoff: "exponential" } + );apps/web/utils/gmail/message.ts (1)
27-50: LGTM! Well-implemented message retrieval with proper error handling.The function correctly:
- Cleans RFC822 Message-ID by removing angle brackets
- Uses appropriate Gmail API parameters
- Handles missing messages gracefully
Consider adding JSDoc documentation to clarify the function's purpose and parameters.
+/** + * Retrieves a Gmail message using its RFC822 Message-ID. + * @param rfc822MessageId - The RFC822 Message-ID to search for + * @param gmail - The Gmail API client + * @returns The message if found, null otherwise + */ export async function getMessageByRfc822Id(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/(app)/onboarding-old2/page.tsx(0 hunks)apps/web/app/api/google/webhook/process-history.ts(10 hunks)apps/web/utils/ai/assistant/process-user-request.ts(1 hunks)apps/web/utils/assistant/get-original-message-id.ts(1 hunks)apps/web/utils/assistant/is-assistant-email.test.ts(1 hunks)apps/web/utils/assistant/is-assistant-email.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)apps/web/utils/gmail/message.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/(app)/onboarding-old2/page.tsx
🔇 Additional comments (7)
apps/web/utils/assistant/is-assistant-email.test.ts (2)
4-11: LGTM! Core test case validates assistant email format.The test correctly verifies that an email in the format
user+assistant@domain.comis recognized as an assistant email.
13-19: LGTM! Negative test case prevents false positives.The test ensures that assistant emails from different users are not mistakenly identified.
apps/web/utils/assistant/process-assistant-email.ts (3)
10-10: Address the TODO comment for loading full email thread history.The TODO indicates missing functionality for loading the complete email thread history, which could be important for context in processing assistant emails.
Would you like me to help implement the thread history loading functionality or create an issue to track this task?
21-28: LGTM! Robust email verification prevents unauthorized access.The verification logic correctly checks sender authenticity using case-insensitive comparison.
47-49: Implement the processUserRequest functionality.The TODO comment indicates missing implementation for processing user requests.
Would you like me to help implement the user request processing functionality or create an issue to track this task?
apps/web/app/api/google/webhook/process-history.ts (2)
357-369: LGTM! Well-integrated assistant email handling.The integration correctly:
- Checks for assistant emails
- Logs the skip reason
- Processes assistant emails appropriately
317-326: LGTM! Proper message processing lock prevents duplicate processing.The implementation correctly uses a lock to prevent processing the same message multiple times.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/utils/assistant/is-assistant-email.test.ts (1)
4-44: Well-structured test suite with comprehensive coverage!The test cases effectively cover various email formats and scenarios, including basic validation, negative cases, and complex email patterns.
Consider adding these test cases for enhanced coverage:
- Multiple recipients (To field with multiple addresses)
- CC/BCC scenarios
- Invalid email formats
- Case sensitivity checks
- Multiple + segments (e.g., user+foo+assistant@example.com)
// Example additional test cases: it("should handle multiple recipients", () => { const result = isAssistantEmail({ userEmail: "john@example.com", recipientEmail: "jane@example.com, john+assistant@example.com", }); expect(result).toBe(true); }); it("should be case insensitive", () => { const result = isAssistantEmail({ userEmail: "John@Example.com", recipientEmail: "JOHN+ASSISTANT@EXAMPLE.COM", }); expect(result).toBe(true); });apps/web/app/api/google/webhook/process-history.ts (1)
356-368: Well-placed assistant email check with early return.The assistant email check is strategically placed before other processing steps, which is efficient. However, consider adding logging for cases where the assistant email processing fails.
- return processAssistantEmail({ message, userEmail, gmail }); + try { + return await processAssistantEmail({ message, userEmail, gmail }); + } catch (error) { + logger.error("Failed to process assistant email", { + email: userEmail, + messageId, + threadId, + error, + }); + throw error; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/app/api/google/webhook/process-history.ts(11 hunks)apps/web/utils/assistant/is-assistant-email.test.ts(1 hunks)apps/web/utils/assistant/is-assistant-email.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/utils/assistant/is-assistant-email.ts
- apps/web/utils/assistant/process-assistant-email.ts
🔇 Additional comments (3)
apps/web/app/api/google/webhook/process-history.ts (3)
265-265: Good addition of draft email filtering!The explicit check for draft labels helps prevent processing incomplete emails.
300-300: Consistent variable naming improvement.Good refactoring to use
userEmailconsistently throughout the file instead of mixinguserEmail. This improves code clarity and maintainability.Also applies to: 316-316, 320-320, 328-328, 347-347, 363-363, 379-379, 396-396, 431-431, 454-454, 471-471
316-324: Verify message processing lock mechanism.The message processing lock is crucial for preventing duplicate processing. Let's verify its implementation:
✅ Verification successful
Lock mechanism implementation is robust and secure
The Redis-based lock implementation is well-designed with:
- Atomic operations to prevent race conditions
- 5-minute auto-expiry to handle failures
- Proper isolation using user+message compound keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the Redis lock implementation rg -A 5 "markMessageAsProcessing" --type tsLength of output: 1620
Script:
#!/bin/bash # Get the complete implementation of message-processing.ts cat apps/web/utils/redis/message-processing.tsLength of output: 644
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/utils/email.ts (2)
17-23: Consider using a more restrictive email regex pattern.While the current pattern covers common formats, it could be strengthened to prevent edge cases:
- Local part could allow more special characters than necessary
- Domain part could be more strictly validated
Consider using a more comprehensive pattern:
-const emailPattern = /[a-zA-Z0-9.+]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/i; +const emailPattern = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/i;
25-33: LGTM! Robust two-step email extraction.Good implementation that:
- Prioritizes bracketed content for formatted emails
- Falls back to raw email extraction
- Properly validates and trims the results
However, consider caching the compiled regex pattern for better performance if this function is called frequently.
+// Cache compiled regex for better performance +const EMAIL_PATTERN = /[a-zA-Z0-9.+]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/i; export function extractEmailAddress(email: string): string { - const emailPattern = /[a-zA-Z0-9.+]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/i; // Try last bracketed content first const match = email.match(/<([^<>]+)>$/); - if (match && emailPattern.test(match[1].trim())) { + if (match && EMAIL_PATTERN.test(match[1].trim())) { return match[1].trim(); } // Fall back to finding any email in the string - const rawMatch = email.match(emailPattern); + const rawMatch = email.match(EMAIL_PATTERN); return rawMatch ? rawMatch[0] : ""; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/api/google/webhook/process-history.ts(11 hunks)apps/web/utils/email.test.ts(1 hunks)apps/web/utils/email.ts(1 hunks)
🔇 Additional comments (7)
apps/web/utils/email.test.ts (3)
47-51: LGTM! Good security consideration for nested angle brackets.The test case ensures that only the last valid email is extracted, which helps prevent potential email spoofing attempts.
53-55: LGTM! Comprehensive validation of error cases.Excellent coverage of malformed inputs, including:
- Malformed angle brackets
- Mixed valid/invalid emails
- Empty brackets
- Multiple @ symbols
- Invalid email formats
These tests help ensure robust input validation and prevent potential security issues.
Also applies to: 57-61, 63-73
75-79: LGTM! Good fallback behavior test.The test validates that the function can extract a valid email even when bracketed emails are invalid, providing a robust fallback mechanism.
apps/web/app/api/google/webhook/process-history.ts (4)
25-26: LGTM! Clean import additions.The new assistant-related utility imports are well-organized and follow the project's module structure.
265-265: LGTM! Improved message filtering logic.The separation of SENT message filtering from inbox filtering is a good improvement. It allows assistant emails to be processed even when they are in the SENT folder while maintaining the original filtering for other emails.
Also applies to: 370-371
319-321: LGTM! Consistent logging improvements.The standardization of using
userEmailparameter in logging statements improves code consistency and maintainability.Also applies to: 327-329, 346-349, 362-366, 381-384, 398-401, 433-436, 456-459, 473-476
356-359: Verify assistant email pattern usage across the codebase.Let's ensure consistent usage of the assistant email pattern across the codebase.
✅ Verification successful
Assistant email pattern implementation is consistent and well-structured
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of isAssistantEmail function # and potential hardcoded assistant email patterns # Check for isAssistantEmail function usage echo "Checking isAssistantEmail function usage:" rg "isAssistantEmail" # Check for potential hardcoded assistant email patterns echo "Checking for potential hardcoded assistant email patterns:" rg -i "assistant.*@|@.*assistant" # Check for any TODOs related to assistant emails echo "Checking for assistant-related TODOs:" rg -i "todo.*assistant|assistant.*todo"Length of output: 2687
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
apps/web/utils/ai/assistant/process-user-request.ts (1)
70-72:⚠️ Potential issueRisk of prompt injection via user-provided content.
The
userEmailForLLM.contentis directly interpolated into the prompt without sanitization.Consider implementing input sanitization or validation before using user-provided content in the prompt.
apps/web/app/api/google/webhook/process-history.ts (1)
363-380:⚠️ Potential issueAdd error handling for assistant email processing.
While the assistant email processing logic is well-structured, it lacks error handling.
Wrap the
processAssistantEmailcall in a try-catch block to handle potential failures gracefully.
🧹 Nitpick comments (2)
apps/web/utils/assistant/process-assistant-email.ts (1)
28-35: Consider enhancing error logging.While the error handling is good, consider adding more context to the error message such as message ID and thread ID.
- throw new Error("Unauthorized assistant access attempt"); + throw new Error(`Unauthorized assistant access attempt for message ${message.id} in thread ${message.threadId}`);apps/web/utils/ai/assistant/process-user-request.ts (1)
131-131: Consider making maxSteps configurable.The
maxStepsparameter is hardcoded to 1, which might be limiting for complex operations.Consider making this configurable based on the operation complexity or user preferences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/web/__tests__/ai-rule-fix.test.ts(4 hunks)apps/web/app/(app)/automation/ReportMistake.tsx(0 hunks)apps/web/app/api/google/webhook/process-history.ts(12 hunks)apps/web/utils/actions/ai-rule.ts(1 hunks)apps/web/utils/actions/rule.ts(2 hunks)apps/web/utils/ai/assistant/process-user-request.ts(1 hunks)apps/web/utils/ai/rule/rule-fix.ts(2 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)apps/web/utils/assistant/reply.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/(app)/automation/ReportMistake.tsx
👮 Files not reviewed due to content moderation or server errors (1)
- apps/web/utils/actions/ai-rule.ts
🔇 Additional comments (5)
apps/web/utils/assistant/reply.ts (1)
2-2: Implement the TODO comment.The TODO comment indicates incomplete implementation. What's the expected behavior for this function?
Let me check the codebase for any related implementations or tests:
Would you like me to help implement this function based on the findings?
apps/web/utils/assistant/process-assistant-email.ts (2)
15-16: TODO comment needs to be addressed.The TODO comment indicates that full email thread history loading is not implemented yet.
Do you want me to help implement the thread history loading functionality or create an issue to track this task?
109-114: LGTM! Robust email verification.The
verifyUserSentEmailhelper function correctly handles case-insensitive email comparison.apps/web/utils/ai/assistant/process-user-request.ts (1)
136-139: LGTM! Robust retry mechanism.The retry mechanism with exponential backoff is well implemented:
- Retries only on
InvalidToolArgumentsError- Limited to 3 retries
- 1-second delay between retries
apps/web/app/api/google/webhook/process-history.ts (1)
382-383: LGTM! Clear skip condition for non-assistant sent emails.The condition to skip SENT emails that are not assistant emails is clear and well-placed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/utils/ai/assistant/process-user-request.ts (1)
57-79:⚠️ Potential issueRisk of prompt injection via user-provided content
The
userRequestEmailForLLM.contentis directly interpolated into the prompt without sanitization, which could expose the system to prompt injection attacks.Consider implementing one of these solutions:
- Sanitize user input by escaping special characters and XML-like tags
- Use a structured format (like JSON) instead of XML-like format
- Implement input validation to reject potentially malicious content
Example sanitization function:
function sanitizeForPrompt(content: string): string { return content .replace(/[<>]/g, '') // Remove XML-like tags .replace(/`/g, '\\`') // Escape backticks .trim(); }
🧹 Nitpick comments (2)
apps/web/utils/gmail/mail.ts (1)
99-114: Consider adding logging and rate limiting.To improve operational visibility and API quota management:
- Add logging for debugging and monitoring email replies
- Consider implementing rate limiting to respect Gmail API quotas
Example implementation:
import { logger } from "@/utils/logger"; import { RateLimiter } from "@/utils/rate-limiter"; const emailRateLimiter = new RateLimiter({ maxRequests: 100, // adjust based on your Gmail API quota perSeconds: 60 }); export async function replyToEmail( gmail: gmail_v1.Gmail, message: ParsedMessage, reply: string, ) { await emailRateLimiter.acquire(); try { logger.info("Replying to email", { threadId: message.threadId, subject: message.headers.subject }); const result = await sendEmail(/*...*/); logger.info("Successfully sent reply", { threadId: message.threadId, messageId: result.data.id }); return result; } catch (error) { logger.error("Failed to send reply", { threadId: message.threadId, error }); throw error; } }apps/web/utils/ai/assistant/process-user-request.ts (1)
140-140: Consider increasing maxSteps limitThe current
maxSteps: 2might be too restrictive for complex scenarios where the assistant needs multiple interactions to complete the task.Consider increasing it to 3-5 steps to allow for more complex interactions while still maintaining a reasonable limit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/utils/ai/assistant/process-user-request.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)apps/web/utils/gmail/mail.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/assistant/process-assistant-email.ts
🔇 Additional comments (6)
apps/web/utils/gmail/mail.ts (1)
14-14: LGTM!The import of
ParsedMessagetype is appropriate for the new functionality.apps/web/utils/ai/assistant/process-user-request.ts (5)
1-17: LGTM! Imports are well-organized and necessary.All imports are being used and types are properly defined.
18-18: LGTM! Good use of scoped logging.Using a scoped logger enhances traceability and debugging capabilities.
20-37: LGTM! Well-structured schema definitions with clear descriptions.The schemas properly validate the tool parameters and include helpful descriptions for each field.
39-53: LGTM! Well-typed function signature with proper constraints.The parameters are clearly defined with appropriate TypeScript types, making the function interface type-safe and self-documenting.
54-55: LGTM! Clean email content extraction.Good use of utility function to extract email content for LLM processing.
elie222
force-pushed
the
email-assistant
branch
from
811f274 to
4854df8
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
apps/web/utils/ai/assistant/fix-rules.ts (1)
288-336: Ensure Proper Handling of Null or Undefined Properties inruleToXMLFunctionIn the
ruleToXMLfunction, ensure that all optional properties ofrule, such asinstructions,from,to,subject,body,group,groupItems, andcategoryFilters, are appropriately checked fornullorundefinedbefore usage to prevent runtime errors.apps/web/utils/ai/rule/create-prompt-from-rule.ts (1)
12-15: IncorporategroupItemsinto the Prompt CreationThe
groupItemsproperty has been added to theRuleWithRelationstype but is not utilized within thecreatePromptFromRulefunction. Consider incorporatinggroupItemsinto the prompt generation to provide more context about the group's conditions, which can enhance the accuracy of rule processing.apps/web/__tests__/ai-fix-rules.test.ts (2)
409-515: Extract Helper Functions for ReusabilityThe helper functions
getRule,getParsedMessage,getUser,getGroup,getGroupItem, andgetCategoryare defined within this test file. Consider moving these utility functions to a separate module so they can be reused across multiple test files, promoting DRY (Don't Repeat Yourself) principles and improving maintainability.
19-20: Mock External Dependencies More PreciselyThe mocks for
server-onlyand@/utils/gmail/mailare currently stubs. Ensure that these mocks accurately represent the behavior of the actual modules, especially if the tests rely on specific interactions with these dependencies. This will improve the reliability and accuracy of the test outcomes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/__tests__/ai-fix-rules.test.ts(1 hunks)apps/web/utils/ai/assistant/fix-rules.ts(1 hunks)apps/web/utils/ai/rule/create-prompt-from-rule.ts(1 hunks)apps/web/utils/ai/rule/create-rule-schema.ts(1 hunks)apps/web/utils/logger.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/utils/ai/rule/create-rule-schema.ts
🔇 Additional comments (5)
apps/web/utils/logger.ts (2)
59-59: Maintain consistency with trace logging implementation.The same condition change has been applied here, matching the behavior in
createScopedLogger. This maintains consistency across both logging implementations.The security considerations mentioned in the previous comment apply here as well.
43-43: Consider the security implications of enabling trace logging in all non-production environments.The condition change from checking for "development" to checking for "not production" means trace logs will be enabled in any environment that isn't explicitly production (e.g., staging, testing). While this provides more debugging capabilities, it could potentially expose sensitive information in these environments.
Consider:
- Using an explicit environment variable to control trace logging
- Ensuring sensitive data is properly redacted in trace logs
- Adding rate limiting for trace logs to prevent excessive logging
Let's verify the impact of this change across the codebase:
✅ Verification successful
Trace logging usage appears safe for non-production environments
The codebase review shows that trace logs are primarily used for debugging AI operations, rule processing, and email categorization metadata. No sensitive data like passwords, tokens, or raw emails are being logged. The change to enable trace logging in non-production environments is acceptable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for trace logging usage to assess potential sensitive data exposure rg -A 3 "\.trace\("Length of output: 10378
apps/web/utils/ai/assistant/fix-rules.ts (1)
258-268: Clarify the Absence ofexecuteFunction in thereplyToolThe
replytool lacks anexecutefunction, with a comment indicating that invoking it will terminate the agent. Ensure that this behavior is intentional and clearly documented, as it might lead to confusion about the tool's functionality. If the tool is meant to send a reply, consider implementing theexecutefunction accordingly.apps/web/utils/ai/rule/create-prompt-from-rule.ts (1)
Line range hint
16-73: Update Type Definitions for ConsistencyWith the change to use
Prisma.GroupGetPayloadfor thegroupproperty, ensure that all type references and usages ofgroupare consistent throughout the codebase. Verify that thegroupproperty's fields are accessed correctly according to the new type definition to prevent potential type errors.apps/web/__tests__/ai-fix-rules.test.ts (1)
16-26: Ensure Tests Are Executed AppropriatelyThe test suite is conditionally skipped based on the
RUN_AI_TESTSenvironment variable. Verify that this variable is set correctly in the testing environments to ensure that the tests are executed when intended. This helps maintain the reliability of the test coverage over time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/actions/rule.ts (1)
246-251: Consider adding error handlingCurrently, any error thrown by
updateRuleInstructionsAndPromptFileis not caught here. Wrap the call in a try/catch block or handle the error gracefully to provide better user feedback and to ensure partial updates are handled appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/actions/rule.ts(2 hunks)apps/web/utils/rule/prompt-file.ts(1 hunks)
🔇 Additional comments (3)
apps/web/utils/rule/prompt-file.ts (2)
5-15: Validate input argumentsPlease ensure to validate that parameters (
userId,ruleId,instructions, etc.) meet expected conditions (e.g., non-empty strings) to avoid potential runtime issues if any of them is missing or malformed.
16-28: Wrap database operations in a transactionThis is a repeated suggestion from a previous review: all database calls in this function should be atomic. If the second operation fails, the first update should be rolled back to maintain data integrity.
try { return await prisma.$transaction(async (tx) => { const updatedRule = await prisma.rule.update({ where: { id: ruleId, userId }, data: { instructions }, include: { actions: true, categoryFilters: true, group: true }, }); if (currentRule) { await updateRulePromptOnRuleChange(userId, currentRule, updatedRule); } else { await appendRulePrompt(userId, instructions); } return updatedRule; }); } catch (error) { console.error("Failed to update rule instructions:", error); throw new Error("Failed to update rule instructions"); }apps/web/utils/actions/rule.ts (1)
27-27: Import statement looks goodNo issues found with importing
updateRuleInstructionsAndPromptFile.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
apps/web/utils/rule/prompt-file.ts (2)
Line range hint
16-45: Optimize database query and add error handling.Several improvements can be made to this function:
- The database query fetches unnecessary fields (aiModel, aiProvider, aiApiKey).
- Missing error handling for database operations.
- No validation of the updated prompt before saving.
Consider these improvements:
export async function updatePromptFileOnRuleUpdated( userId: string, currentRule: RuleWithRelations, updatedRule: RuleWithRelations, ) { + try { const user = await prisma.user.findUnique({ where: { id: userId }, select: { - email: true, - aiModel: true, - aiProvider: true, - aiApiKey: true, rulesPrompt: true, }, }); if (!user) return; const updatedPrompt = await generatePromptOnUpdateRule({ user, existingPrompt: user.rulesPrompt || "", currentRule, updatedRule, }); + if (!updatedPrompt?.trim()) { + throw new Error('Generated prompt is empty or invalid'); + } await prisma.user.update({ where: { id: userId }, data: { rulesPrompt: updatedPrompt }, }); + } catch (error) { + console.error('Failed to update prompt file:', error); + throw new Error('Failed to update prompt file'); + } }
Line range hint
71-85: Improve error handling and prompt formatting.The function has several areas for improvement:
- Silent return when user/rulesPrompt doesn't exist could hide issues.
- No validation of rulePrompt input.
- String concatenation could lead to formatting issues.
Consider these improvements:
async function appendRulePrompt(userId: string, rulePrompt: string) { + if (!rulePrompt?.trim()) { + throw new Error('Rule prompt cannot be empty'); + } + const user = await prisma.user.findUnique({ where: { id: userId }, select: { rulesPrompt: true }, }); - if (!user?.rulesPrompt) return; + if (!user) { + throw new Error('User not found'); + } + + if (!user.rulesPrompt) { + throw new Error('User rules prompt not initialized'); + } - const updatedPrompt = `${user.rulesPrompt || ""}\n\n* ${rulePrompt}.`.trim(); + // Ensure consistent formatting with double newlines between rules + const updatedPrompt = `${user.rulesPrompt.trim()}\n\n* ${rulePrompt.trim()}.`; await prisma.user.update({ where: { id: userId }, data: { rulesPrompt: updatedPrompt }, }); }
♻️ Duplicate comments (6)
apps/web/utils/rule/rule.ts (3)
34-44:⚠️ Potential issueFix parameter mismatch in retry attempt.
The retry attempt in
safeCreateRuleusescategoryIdsinstead ofcategoryNames, which could lead to incorrect category associations.- categoryIds, + categoryNames,
144-147: 🛠️ Refactor suggestionConsider safer action updates to prevent potential data loss.
The current implementation deletes and recreates all actions. While safe now, this approach could lead to data loss if
Actiongains relations in the future.Consider implementing an upsert pattern that:
- Compares existing vs new actions
- Updates matching actions
- Creates new actions
- Deletes only removed actions
71-80: 🛠️ Refactor suggestionReconsider creating new rules during update.
Creating a new rule on name conflict during update could lead to rule proliferation and user confusion.
Consider alternatives:
- Append a meaningful suffix (e.g., "-copy" instead of timestamp)
- Update the existing rule
- Return an error asking the user to choose a different name
apps/web/utils/ai/assistant/process-user-request.ts (3)
91-123:⚠️ Potential issueMitigate prompt injection risk.
The user input is directly interpolated into the prompt without sanitization, which could expose the system to prompt injection attacks.
Consider implementing input sanitization:
- content: prompt, + content: sanitizeUserInput(prompt), // Add this functionAdd a utility function to sanitize the input:
function sanitizeUserInput(input: string): string { // Remove potential prompt injection patterns return input.replace(/^system:|^assistant:|^user:/gmi, '') .replace(/[<>]/g, ''); // Sanitize XML-like tags }
137-137:⚠️ Potential issueAvoid logging sensitive user data.
Logging entire conversation transcripts may expose sensitive user information (PII).
Consider redacting sensitive information:
- logger.trace("Input", { allMessages }); + logger.trace("Input", { + messageCount: allMessages.length, + roles: allMessages.map(m => m.role) + });
38-459: 🛠️ Refactor suggestionConsider decomposing the processUserRequest function.
The function is handling multiple responsibilities including prompt construction, rule logic, and tool definitions. This makes it harder to maintain and test.
Consider breaking it down into smaller, focused functions:
- Prompt construction
- Tool definitions
- Rule processing logic
Example refactor:
async function processUserRequest(params: ProcessUserRequestParams) { const prompt = constructPrompt(params); const tools = defineTools(params); const result = await processWithTools(prompt, tools); await handleRuleUpdates(result); return result; }
🧹 Nitpick comments (6)
apps/web/utils/rule/rule.ts (3)
168-181: Improve maintainability of automation rules.The
shouldAutomatefunction uses a hardcoded list of action types that prevent automation. This could become difficult to maintain as new action types are added.Consider using a configuration object or enum to define non-automatable actions:
const NON_AUTOMATABLE_ACTIONS = new Set([ ActionType.REPLY, ActionType.FORWARD, ActionType.SEND_EMAIL, ]); function shouldAutomate(actions: Pick<Action, "type">[]) { return !actions.some(action => NON_AUTOMATABLE_ACTIONS.has(action.type)); }
189-226: Refactor duplicate category management logic.The
addRuleCategoriesandremoveRuleCategoriesfunctions share similar structure and error handling.Consider extracting common logic into a helper function:
async function updateRuleCategories( ruleId: string, categoryIds: string[], operation: 'add' | 'remove' ) { const rule = await prisma.rule.findUnique({ where: { id: ruleId }, include: { categoryFilters: true }, }); if (!rule) throw new Error("Rule not found"); const existingIds = rule.categoryFilters.map((c) => c.id) || []; const newIds = operation === 'add' ? [...new Set([...existingIds, ...categoryIds])] : existingIds.filter((id) => !categoryIds.includes(id)); return prisma.rule.update({ where: { id: ruleId }, data: { categoryFilters: { set: newIds.map((id) => ({ id })) } }, include: { actions: true, categoryFilters: true, group: true }, }); }
195-195: Enhance error handling for rule not found scenarios.The error messages could be more informative by including the rule ID.
- if (!rule) throw new Error("Rule not found"); + if (!rule) throw new Error(`Rule not found: ${ruleId}`);Also applies to: 216-216
apps/web/utils/ai/assistant/process-user-request.ts (2)
461-498: Enhance error handling in getUpdateCategoryTool.The function could benefit from more robust error handling, especially around the sender existence check.
Consider this enhancement:
execute: async ({ sender, category }) => { logger.info("Update Category", { sender, category }); const existingSender = await findSenderByEmail({ userId, email: sender, }); + if (!sender.includes('@')) { + return { error: "Invalid email format" }; + } + const cat = categories.find((c) => c.name === category); if (!cat) { logger.error("Category not found", { category }); return { error: "Category not found" }; } + try { await updateCategoryForSender({ userId, sender: existingSender?.email || sender, categoryId: cat.id, }); return { success: true }; + } catch (error) { + logger.error("Failed to update sender category", { error }); + return { + error: "Failed to update sender category", + message: error instanceof Error ? error.message : String(error) + }; + } }
500-554: Add input validation for XML conversion functions.The XML conversion functions don't validate input, which could lead to malformed XML if the input contains special characters.
Consider adding XML escaping:
function escapeXml(unsafe: string): string { return unsafe.replace(/[<>&'"]/g, (c) => { switch (c) { case '<': return '<'; case '>': return '>'; case '&': return '&'; case '\'': return '''; case '"': return '"'; default: return c; } }); }Then use it in the XML generation:
function ruleToXML(rule: RuleWithRelations) { return `<rule> - <rule_name>${rule.name}</rule_name> + <rule_name>${escapeXml(rule.name)}</rule_name> <conditions>apps/web/utils/rule/prompt-file.ts (1)
8-14: Add error handling for database operations.The function should handle potential failures in the database operations to ensure proper error propagation and logging.
Consider wrapping the operations in a try-catch block:
export async function updatePromptFileOnRuleCreated( userId: string, rule: RuleWithRelations, ) { + try { const prompt = createPromptFromRule(rule); await appendRulePrompt(userId, prompt); + } catch (error) { + console.error('Failed to update prompt file:', error); + throw new Error('Failed to update prompt file'); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/utils/actions/rule.ts(4 hunks)apps/web/utils/ai/assistant/process-user-request.ts(1 hunks)apps/web/utils/rule/prompt-file.ts(2 hunks)apps/web/utils/rule/rule.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/actions/rule.ts
🔇 Additional comments (2)
apps/web/utils/rule/rule.ts (1)
1-13: LGTM! Well-structured imports and logger setup.Clean organization of imports with proper type handling and scoped logging setup.
apps/web/utils/ai/assistant/process-user-request.ts (1)
1-36: LGTM! Well-organized imports and type definitions.The imports are properly structured and all seem necessary for the functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
apps/web/utils/assistant/process-assistant-email.ts (4)
27-34:⚠️ Potential issueStrengthen email verification.
The current email verification is case-insensitive but doesn't handle potential email spoofing or header manipulation.
Apply this diff to improve email verification:
function verifyUserSentEmail(message: ParsedMessage, userEmail: string) { + const from = message.headers.from; + if (!from) return false; + + // Verify both email and optional display name + const fromEmail = extractEmailAddress(from).toLowerCase(); + const expectedEmail = userEmail.toLowerCase(); + + // Additional checks for email authenticity + const spf = message.headers['received-spf']; + const dkim = message.headers['dkim-signature']; + return ( - extractEmailAddress(message.headers.from).toLowerCase() === - userEmail.toLowerCase() + fromEmail === expectedEmail && + (spf?.includes('pass') || dkim) ); }
79-144:⚠️ Potential issueAdd type definitions and error handling for database operations.
The Promise.all operation could fail if any of the database queries throw an error.
Apply this diff to add type definitions and error handling:
+type UserSelect = NonNullable<Awaited<ReturnType<typeof prisma.user.findUnique>>> +type ExecutedRuleSelect = NonNullable<Awaited<ReturnType<typeof prisma.executedRule.findUnique>>> +type NewsletterSelect = NonNullable<Awaited<ReturnType<typeof prisma.newsletter.findUnique>>> - const [user, executedRule, senderCategory] = await Promise.all([ + let user: UserSelect | null = null; + let executedRule: ExecutedRuleSelect | null = null; + let senderCategory: NewsletterSelect | null = null; + + try { + [user, executedRule, senderCategory] = await Promise.all([ prisma.user.findUnique({ // ... existing query }), prisma.executedRule.findUnique({ // ... existing query }), prisma.newsletter.findUnique({ // ... existing query }), ]); + } catch (error) { + logger.error("Database query failed", { error, messageId: message.id }); + await replyToEmail( + gmail, + message, + "I encountered an error while processing your request. Please try again later." + ); + return; + }
182-202:⚠️ Potential issueAdd error handling for processUserRequest.
The function should handle potential errors from processUserRequest to ensure graceful failure.
Apply this diff to add error handling:
- const result = await processUserRequest({ + let result; + try { + result = await processUserRequest({ user, rules: user.rules, userRequestEmail: message, originalEmail: parsedOriginalMessage, matchedRule: executedRule?.rule || null, categories: user.categories.length ? (user.categories.map((c) => c.name) as [string, ...string[]]) : null, senderCategory: senderCategory?.category?.name ?? null, gmail, }); + } catch (error) { + logger.error("Failed to process user request", { error, messageId: message.id }); + await replyToEmail( + gmail, + message, + "I encountered an error while processing your request. Please try again later.", + ); + return; + }
217-225:⚠️ Potential issueAdd error handling for message retrieval and parsing.
The function should handle potential errors from
getMessageByRfc822IdandparseMessage.Apply this diff to add error handling:
async function getOriginalMessage( originalMessageId: string | undefined, gmail: gmail_v1.Gmail, ) { if (!originalMessageId) return null; - const originalMessage = await getMessageByRfc822Id(originalMessageId, gmail); - if (!originalMessage) return null; - return parseMessage(originalMessage); + try { + const originalMessage = await getMessageByRfc822Id(originalMessageId, gmail); + if (!originalMessage) return null; + return parseMessage(originalMessage); + } catch (error) { + logger.error("Failed to retrieve or parse original message", { + error, + originalMessageId, + }); + return null; + } }apps/web/utils/ai/assistant/process-user-request.ts (3)
92-124:⚠️ Potential issueRisk of prompt injection via user-provided content.
The prompt construction directly interpolates user-provided content without sanitization, which could expose the system to prompt injection attacks.
Consider implementing a sanitization function:
function sanitizePromptContent(content: string): string { // Remove potential XML/prompt injection patterns return content .replace(/<\/?[^>]+(>|$)/g, "") // Remove XML tags .replace(/^system:/i, "") // Remove system: prefix .replace(/^assistant:/i, "") // Remove assistant: prefix .replace(/^user:/i, ""); // Remove user: prefix }Then apply this diff to use the sanitization:
- ${user.about ? `<user_about>${user.about}</user_about>` : ""} + ${user.about ? `<user_about>${sanitizePromptContent(user.about)}</user_about>` : ""} - ${originalEmail ? stringifyEmailSimple(getEmailForLLM(originalEmail)) : ""} + ${originalEmail ? sanitizePromptContent(stringifyEmailSimple(getEmailForLLM(originalEmail))) : ""}
430-437:⚠️ Potential issueAdd error handling for replyToEmail.
The reply tool lacks error handling.
Apply this diff to add error handling:
reply: tool({ description: "Send an email reply to the user", parameters: z.object({ content: z.string().describe("The content of the reply"), }), - // no execute function - invoking it will terminate the agent + execute: async ({ content }) => { + try { + return await replyToEmail(gmail, message, content); + } catch (error) { + logger.error("Failed to send reply email", { error }); + throw new Error("Failed to send reply email"); + } + } }),
138-138:⚠️ Potential issueAvoid logging sensitive information.
Logging entire conversation transcripts and tool calls may expose sensitive user information (PII).
Consider redacting or selectively logging only the necessary details:
- logger.trace("Input", { allMessages }); + logger.trace("Input", { + messageCount: allMessages.length, + hasSystemPrompt: allMessages[0]?.role === "system", + hasUserPrompt: allMessages[1]?.role === "user", + }); - logger.trace("Tool Calls", { toolCalls }); + logger.trace("Tool Calls", { + count: toolCalls.length, + tools: toolCalls.map(call => call.toolName), + });Also applies to: 445-445
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/ai/assistant/process-user-request.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)
🔇 Additional comments (2)
apps/web/utils/assistant/process-assistant-email.ts (1)
212-215: LGTM!The function correctly handles email address formatting.
apps/web/utils/ai/assistant/process-user-request.ts (1)
1-568: Consider decomposing the file into smaller modules.The file handles multiple concerns and could benefit from being split into focused modules:
- Prompt construction
- Tool definitions
- Rule processing
- XML conversion
Consider creating separate modules:
prompt.tsfor prompt constructiontools/directory for individual toolsrule-processor.tsfor rule processing logicxml-converter.tsfor XML conversionThis would improve maintainability, readability, and testability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/actions/group.ts (1)
353-366: Consider using batch upsert for better efficiency.While the current implementation with deduplication and graceful fallback is good, consider using
prisma.groupItem.upsertin a transaction for better efficiency. This would handle duplicates in a single database operation.const uniqueItems = uniqBy(data, (item) => `${item.value}-${item.type}`); try { - return await prisma.groupItem.createMany({ data: uniqueItems }); + return await prisma.$transaction( + uniqueItems.map((item) => + prisma.groupItem.upsert({ + where: { + groupId_type_value: { + groupId: item.groupId, + type: item.type, + value: item.value, + }, + }, + create: item, + update: {}, // No update needed if exists + }) + ) + ); } catch (error) { - if (isDuplicateError(error)) { - // Create items one by one, skipping duplicates - for (const item of uniqueItems) { - try { - await prisma.groupItem.create({ data: item }); - } catch (error) { - if (!isDuplicateError(error)) throw error; - } - } - } - - throw error; + throw new Error(`Failed to create group items: ${error.message}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/actions/group.ts(4 hunks)apps/web/utils/group/group-item.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/group/group-item.ts
🔇 Additional comments (3)
apps/web/utils/actions/group.ts (3)
30-30: LGTM! Good separation of concerns.Moving group item operations to a dedicated module improves code organization and maintainability.
414-414: LGTM! Good use of the utility function.The change maintains proper permission checks while improving code organization.
426-426: LGTM! Good use of the utility function.The change maintains proper authentication checks while improving code organization.
1 participant
Summary by CodeRabbit
Based on the comprehensive pull request summary, here are the updated high-level release notes:
New Features
Improvements
Bug Fixes
Technical Updates
node-email-reply-parsertoemail-reply-parser.