Learned patterns #395
Learned patterns #395
Conversation
…ion formatting. Update response structure to use rule names instead of numbers for better clarity.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request revises how AI rules are defined and applied across the application. The rules schema is updated to include both a name and instructions, and rule lookup now uses names instead of numeric indices. The changes extend to several test suites that now conditionally run based on an environment flag. Additional functionality is introduced with new files for detecting recurring patterns and analyzing sender behavior. Other updates include modifying imports, environment configurations, and cleanup of dependency and conditional checks. Changes
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/web/app/api/ai/analyze-sender-pattern/route.tsOops! Something went wrong! :( ESLint: 9.23.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.tsOops! Something went wrong! :( ESLint: 9.23.0 ESLint couldn't find an eslint.config.(js|mjs|cjs) file. From ESLint v9.0.0, the default configuration file is now eslint.config.js. https://eslint.org/docs/latest/use/configure/migration-guide If you still have problems after following the migration guide, please stop by ✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts (1)
147-151: Improved rule selection with case-insensitive name matchingThe rule selection logic now finds rules by name instead of index, with case-insensitive matching to increase robustness. This is a good improvement over the previous approach.
However, this doesn't handle the edge case where multiple rules could have the same name (when compared case-insensitively).
Consider adding a check or warning if multiple rules have names that are the same when compared case-insensitively:
const selectedRule = aiResponse.ruleName - ? rules.find( - (rule) => rule.name.toLowerCase() === aiResponse.ruleName.toLowerCase(), - ) + ? (() => { + const normalizedRuleName = aiResponse.ruleName.toLowerCase(); + const matchingRules = rules.filter( + (rule) => rule.name.toLowerCase() === normalizedRuleName + ); + + if (matchingRules.length > 1) { + logger.warn("Multiple rules with the same name found", { + ruleName: aiResponse.ruleName, + matchingRuleCount: matchingRules.length + }); + } + + return matchingRules[0]; + })() : undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts(5 hunks)apps/web/utils/reply-tracker/inbound.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts (1)
apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)
🔇 Additional comments (10)
apps/web/utils/reply-tracker/inbound.ts (1)
134-136: Added rule name to aiChooseRule argumentsThis change correctly adds the rule name to the object passed to
aiChooseRule, which is necessary since the function now selects rules by name instead of index. This aligns with the updated function signature inai-choose-rule.ts.apps/web/utils/ai/choose-rule/ai-choose-rule.ts (9)
16-16: Updated type definition to include rule nameThe type has been correctly updated to require both
nameandinstructionsfor each rule, which is necessary for the name-based rule selection approach.
27-40: Improved AI instructions for rule selectionThe instructions have been significantly enhanced with a clear priority list and detailed guidelines. This structured approach should help the AI make more accurate rule selections.
44-51: Updated rule formatting to include rule namesThe XML-like format now properly includes the rule name, making it clear to the AI which rule is which. This is a necessary change to support the name-based selection approach.
55-59: Added name to system fallback ruleThe system fallback rule now has a name for consistency with the user-defined rules. This ensures the fallback can be selected using the same name-based approach.
75-75: Updated output format instructionChanged from requesting a rule number to requesting a rule name, which aligns with the new selection approach.
108-108: Updated schema validation for rule nameThe schema correctly expects a string rule name instead of a number, matching the new approach.
120-123: Updated Braintrust logging to include rule namesThe data logging now includes rule names, ensuring consistency with the rest of the changes.
128-128: Updated expected value in Braintrust datasetNow correctly expects the rule name instead of a rule number.
135-135: Updated generic type constraintThe generic type constraint now correctly requires both
nameandinstructionsproperties.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts (1)
141-148: Consider handling duplicate rule namesThe rule selection logic now correctly matches by name (case-insensitive), but there's no protection against duplicate rule names. If multiple rules have the same name when converted to lowercase, the first matching rule will be selected.
Consider adding validation to ensure rule names are unique (case-insensitive) or adding logic to handle duplicate rule names, such as:
// Validation function that could be added + function validateUniqueRuleNames(rules: Array<{ name: string }>): void { + const lowerCaseNames = new Set<string>(); + for (const rule of rules) { + const lowerName = rule.name.toLowerCase(); + if (lowerCaseNames.has(lowerName)) { + console.warn(`Duplicate rule name detected: "${rule.name}"`); + } + lowerCaseNames.add(lowerName); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts(5 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts (4)
apps/web/utils/braintrust.ts (1)
Braintrust(4-22)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)
🔇 Additional comments (8)
apps/web/utils/ai/choose-rule/ai-choose-rule.ts (8)
11-11: Dataset version update noted.The dataset name change from likely "choose-rule" to "choose-rule-2" reflects the schema changes for named rules rather than rule numbers.
16-16: Good enhancement: Adding name property to rulesAdding the
nameproperty to rules improves self-documentation and enables more precise rule selection by matching on names rather than indices.
27-40: Well-structured guidance for rule selectionThe enhanced instructions with clear priority order and specific guidelines will help the AI make more consistent and appropriate rule selections. The hierarchical structure and explicit examples improve the AI's understanding of the selection criteria.
43-50: Rule format properly updated for named rulesThe rule format in the prompt now correctly includes both name and instructions for each rule, giving the AI proper context for rule selection.
67-68: Improved output format with explicit rule identificationChanging from rule number to rule name makes the output more explicit and less error-prone. The addition of the
noMatchFoundfield provides clearer handling for cases where no rules match.
101-103: Schema properly updated to match new output formatThe schema definition now correctly validates the new output format with
ruleNameas a string and optionalnoMatchFoundboolean.
114-122: Braintrust dataset insertion properly updatedThe data inserted into Braintrust now correctly includes rule names and uses the rule name as the expected output, maintaining alignment with the new approach.
129-130: Type constraint properly updated for rule structureThe type constraint for the generic parameter has been correctly updated to require both
nameandinstructionsproperties.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
110-125: Consider enabling the Braintrust dataset collectionThere's commented code for inserting data to a Braintrust dataset. Consider either enabling this feature for model improvement or removing the commented code if it's not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.cursor/rules/llm-test.mdc(1 hunks)apps/web/__tests__/ai-categorize-senders.test.ts(1 hunks)apps/web/__tests__/ai-choose-args.test.ts(1 hunks)apps/web/__tests__/ai-choose-rule.test.ts(1 hunks)apps/web/__tests__/ai-create-group.test.ts(1 hunks)apps/web/__tests__/ai-detect-recurring-pattern.test.ts(1 hunks)apps/web/__tests__/ai-diff-rules.test.ts(1 hunks)apps/web/__tests__/ai-example-matches.test.ts(1 hunks)apps/web/__tests__/ai-extract-from-email-history.test.ts(1 hunks)apps/web/__tests__/ai-extract-knowledge.test.ts(1 hunks)apps/web/__tests__/ai-find-snippets.test.ts(1 hunks)apps/web/__tests__/ai-prompt-to-rules.test.ts(1 hunks)apps/web/__tests__/ai-rule-fix.test.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/__tests__/ai-detect-recurring-pattern.test.ts (3)
apps/web/utils/rule/consts.ts (1)
RuleName(2-9)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
detectRecurringPattern(23-136)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
🔇 Additional comments (23)
apps/web/__tests__/ai-example-matches.test.ts (1)
17-17: Improved test execution conditionThis change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)makes the test execution condition more direct and easier to understand. Both expressions are logically equivalent, but the new version better expresses the intent to run tests only when AI tests are enabled.apps/web/__tests__/ai-extract-from-email-history.test.ts (1)
45-45: Improved test execution conditionThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)is a semantic improvement that makes the code more readable. It directly expresses the intent to run tests when AI testing is enabled rather than skipping them when it's not.apps/web/__tests__/ai-find-snippets.test.ts (1)
10-10: Improved test execution conditionChanging from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)is a positive refactoring that improves readability by expressing the condition positively. This change aligns with the same pattern applied across other test files in this PR.apps/web/__tests__/ai-rule-fix.test.ts (1)
12-12: Improved test execution conditionThis change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)maintains the same conditional test execution logic while making the intent clearer. This consistent approach across all test files improves code maintainability.apps/web/__tests__/ai-categorize-senders.test.ts (1)
54-54: Great improvement on test conditional executionChanging from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)improves readability by making the condition more straightforward. This approach is more aligned with Vitest's intended API usage and makes the test's execution condition clearer.apps/web/__tests__/ai-choose-args.test.ts (1)
12-12: Improved test conditional execution syntaxThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)provides a more direct and readable way to express when the tests should run. This makes the code's intent clearer and follows a more positive condition pattern.apps/web/__tests__/ai-prompt-to-rules.test.ts (1)
12-12: Better test conditional execution approachChanging from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)is a good improvement that follows the positive condition pattern. This makes the code more readable and consistent with the other test files in the codebase.apps/web/__tests__/ai-choose-rule.test.ts (1)
12-12: Enhanced test execution condition clarityThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)improves code readability by using a positive condition. This is also more consistent with the updates made to other test files, creating a uniform pattern across the test suite.apps/web/__tests__/ai-diff-rules.test.ts (1)
10-10: Improved test execution logicThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)provides clearer intent and better readability, while maintaining the same conditional execution logic. This aligns with similar changes across other test files in the project.apps/web/__tests__/ai-extract-knowledge.test.ts (1)
94-94: Improved test execution logicThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)provides clearer intent and better readability, while maintaining the same conditional execution logic. This modification is consistent with the project-wide effort to standardize test execution conditions.apps/web/__tests__/ai-create-group.test.ts (1)
16-16: Improved test execution logicThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)provides clearer intent and better readability, while maintaining the same conditional execution logic. This modification is consistent with the project-wide effort to standardize test execution conditions..cursor/rules/llm-test.mdc (2)
34-34: Improved test template execution logicThe change from
describe.skipIf(!isAiTest)todescribe.runIf(isAiTest)in the template provides clearer intent and better readability, while maintaining the same conditional execution logic. This update ensures that new tests created from this template will follow the standardized approach.
42-42: Added appropriate timeout for LLM testsAdding the 15-second timeout parameter to the test suite is a good practice for LLM-related tests, which can take longer to complete than standard unit tests. This ensures tests don't run indefinitely in case of issues and aligns with the best practices mentioned later in the document (line 105).
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (4)
1-16: Clean implementation of schema and typesThe schema definition using Zod is well-structured and appropriately exported as a type. The nullable
matchedRulefield correctly handles the case when no matching rule is found.
36-82: Well-structured prompt with clear instructionsThe system prompt is well-organized with clear instructions and examples. The structure using XML tags will help the AI model parse different sections effectively.
84-96: Concise prompt construction with effective use of stringifyEmail utilityThe prompt construction is concise and effective, making good use of the stringifyEmail utility to format the email content within a reasonable length limit.
126-135: Good error handling and response validationThe function correctly handles errors and validates the AI response, returning null in both cases where the matchedRule is null or an error occurs.
apps/web/__tests__/ai-detect-recurring-pattern.test.ts (6)
1-20: Well-structured test setup with appropriate mocksThe test setup properly mocks necessary dependencies and creates a class for dataset insertion. The use of
vi.clearAllMocks()inbeforeEachensures tests don't affect each other.
21-23: Good use of conditional testing based on environment variableUsing the environment variable
RUN_AI_TESTSto conditionally run AI tests is a good practice, as it avoids running potentially expensive or time-consuming AI tests during regular CI runs.
42-82: Well-defined rule structure with named rulesThe test correctly defines rules with both
nameandinstructionsfields, aligning with the PR objective of using rule names instead of rule numbers. The use ofRuleNameconstants from the imports ensures consistency.
84-229: Comprehensive test data generation functionsThe functions to generate test data for different email types are well-structured and create realistic email scenarios. This helps ensure the test cases accurately reflect real-world usage.
231-307: Comprehensive test coverage for different scenariosThe tests cover a wide range of scenarios, including positive cases (newsletters, receipts, calendar invites, replies) and negative cases (mixed inconsistent emails). Each test has clear assertions and debug logging.
308-310: Appropriate timeout setting for AI-based testsThe 15-second timeout for the test suite is reasonable for tests that involve AI model calls, which may take longer than standard unit tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.cursor/rules/gmail-api.mdc (3)
1-5: Consider specifying glob patterns if this rule should target specific files.The
globsfield is currently empty. If this rule should apply to specific file patterns (e.g., files that interact with Gmail API), consider adding appropriate glob patterns here to target those files.--- description: Guidelines for working with Gmail API globs: + globs: ["**/utils/gmail/**/*.ts", "**/pages/api/**/*.ts"] alwaysApply: false ---
65-68: Consider expanding the "Why This Matters" section.While the point about future provider support is crucial, this section could be expanded to include additional benefits such as:
- Testability: Abstraction layers make it easier to mock API calls for testing
- Error Handling: Centralized error handling and retries
- Rate Limiting: Managing API quotas and rate limits consistently
- Performance Optimization: Opportunities for caching and batching requests
These additional points would strengthen the case for following these guidelines.
1-68: Consider adding sections on error handling and authentication.The document provides excellent guidance on code organization and abstraction, but it could benefit from addressing two critical aspects of API interaction:
- Error Handling: Guidelines for consistent error handling, retries, and reporting
- Authentication: Best practices for managing API credentials and tokens
These additions would make the guidelines more comprehensive and practical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.cursor/rules/gmail-api.mdc(1 hunks)
🔇 Additional comments (5)
.cursor/rules/gmail-api.mdc (5)
8-10: This is a well-structured introduction to the API usage guidelines.The introduction clearly establishes the purpose of these guidelines and acknowledges the current state of provider support. This context is valuable for developers working with the codebase.
11-16: Good core principles for API abstraction.These principles establish a solid foundation for maintaining clean separation of concerns and enabling future extensibility for additional email providers.
17-28: Clear and helpful directory structure guidance.The directory structure example provides developers with a concrete reference for organizing code related to email provider APIs, which will help maintain consistency across the codebase.
30-47: Well-illustrated example of proper API usage.The "DO" example effectively demonstrates the recommended approach of using abstraction layers, with good comments explaining the rationale.
49-63: Clear demonstration of anti-pattern to avoid.The "DON'T" example clearly illustrates what practices to avoid when working with provider APIs, emphasizing the maintenance challenges that direct API calls would create.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
10-10: Remove unnecessary commented-out code.
The commented-outBraintrustimport or usage can be removed if not needed to keep the codebase clean.- // const braintrust = new Braintrust("recurring-pattern-detection");
110-121: Remove or finalize commented-out Braintrust code.
Leaving this large block of commented-out logic may cause confusion. If planning future use, add a note; otherwise remove it.apps/web/app/api/ai/pattern-match/route.ts (3)
30-30: Consider making the email threshold configurable.
HardcodingTHRESHOLD_EMAILS = 3could be restrictive if requirements change.
118-124: Implement or remove thesaveToDbfunction.
This function currently has aTODOwithout any logic. Consider adding the necessary database operations or removing it if not needed.Do you want me to generate a basic Prisma-based implementation or open an issue to track this?
155-200: Add fallback handling for missing Gmail message headers.
Ifmessage.headersis missing certain fields (e.g.,subject,from), this code may break. Consider defensive checks before mapping values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/__tests__/ai-detect-recurring-pattern.test.ts(1 hunks)apps/web/app/api/ai/pattern-match/route.ts(1 hunks)apps/web/app/api/google/webhook/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/api/google/webhook/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/tests/ai-detect-recurring-pattern.test.ts
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
apps/web/app/api/ai/pattern-match/route.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
aiDetectRecurringPattern(23-136)DetectPatternResult(16-16)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/date.ts (1)
internalDateToDate(46-53)
🔇 Additional comments (4)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
1-9: Imports and logger initialization look good.
No concerns in this section.
12-16: Zod schema definition is appropriate.
This schema looks properly defined for the expected fields. If you need tighter control, consider setting minimum length constraints on theexplanationfield.apps/web/app/api/ai/pattern-match/route.ts (2)
24-28: Zod schema usage is nicely constrained.
The defaults and validation rules look good forsenderandmaxResults.
91-95:saveToDbusage but no implementation.
This block callssaveToDbeven though the function body is still pending. Ensure the final implementation is completed or remove the call.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
30-33: Validate all emails are from the same sender.
You already assume a single sender by extracting thefromfield from the first email. For code safety, confirm that each email inemailshas the same sender to avoid incorrect assumptions.+ // Ensure all emails share the same sender + const allSameSender = emails.every((msg) => msg.from === senderEmail); + if (!allSameSender) { + logger.warn("Emails from different senders provided to aiDetectRecurringPattern"); + return null; + }
🧹 Nitpick comments (6)
apps/web/__tests__/ai-detect-recurring-pattern.test.ts (3)
8-19: Consider removing or limiting debug logs in tests.Excessive logging (trace, console.debug) within test code may clutter terminal output, making it harder to trace critical test failures. Consider removing these statements or switching them to minimal logs unless they are essential for debugging.
31-82: Consider refactoring repetitive email generator logic.Several helper functions (e.g.,
getNewsletterEmails,getReceiptEmails, etc.) use similar patterns for generating test emails. You could unify them into a single reusable generator function to reduce code repetition.- function getNewsletterEmails(): EmailForLLM[] { - return Array.from({ length: 7 }).map((_, i) => ({ - // ... - })); - } - function getReceiptEmails(): EmailForLLM[] { - return Array.from({ length: 6 }).map((_, i) => ({ - // ... - })); - } + function generateTestEmails( + length: number, + from: string, + subjectFactory: (i: number) => string, + contentFactory: (i: number) => string, + ): EmailForLLM[] { + return Array.from({ length }).map((_, i) => ({ + id: `${from}-${i}`, + from, + subject: subjectFactory(i), + content: contentFactory(i), + date: new Date(Date.now() - i * 24 * 60 * 60 * 1000), + })); + }
283-293: Expand tests for edge cases with fewer than 3 emails.Although there is coverage for mixed, inconsistent emails, consider explicitly testing scenarios where only 1 or 2 emails are provided, ensuring the function returns null in those cases.
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
36-57: Ensure the prompt clarifies usage of optional typed data.The instructions mention being conservative about returning a matched rule. Consider clarifying how partial or inconsistent email data (e.g., missing subjects) should be handled.
apps/web/app/api/ai/pattern-match/route.ts (2)
64-72: Consider returning more detailed response for insufficient emails.When fewer than three emails are found, clarifying which threshold is unmet (e.g., needed 3 but found N) could help inform the user about how many are required.
91-96: Implement database save logic.Currently,
saveToDbis a placeholder. Persisting thematchedRuleandexplanation(if any) can help with future analytics and auditing.Would you like help drafting the implementation to store the result in a
patternsor similar table?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/__tests__/ai-detect-recurring-pattern.test.ts(1 hunks)apps/web/app/api/ai/pattern-match/route.ts(1 hunks)apps/web/app/api/google/webhook/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/api/google/webhook/route.ts
🧰 Additional context used
🧬 Code Definitions (3)
apps/web/__tests__/ai-detect-recurring-pattern.test.ts (3)
apps/web/utils/rule/consts.ts (1)
RuleName(2-9)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(23-136)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
apps/web/app/api/ai/pattern-match/route.ts (3)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
aiDetectRecurringPattern(23-136)DetectPatternResult(16-16)apps/web/utils/types.ts (1)
EmailForLLM(104-112)
🔇 Additional comments (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
126-129: Great null-check strategy for unmatched rules.Returning
nullifmatchedRuleis not defined ensures the function behaves gracefully and avoids ambiguous or partial returns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/app/api/ai/pattern-match/route.ts (1)
155-157: [Duplicate from previous comment] Consolidate duplicated logic.You have a TODO referencing similar functions in other files. This replicates sender-based retrieval logic used elsewhere. Factor out the common functionality into a shared utility to keep code DRY.
🧹 Nitpick comments (5)
apps/web/app/api/ai/pattern-match/route.ts (5)
1-3: Remove duplicate imports from "next/server".You're importing both
NextResponseandafterfrom "next/server" across lines 1 and 3. For clarity and consistency, consider combining them into a single import statement.-import { NextResponse } from "next/server"; -import { after } from "next/server"; +import { NextResponse, after } from "next/server";
30-31: Optimize threshold usage.
THRESHOLD_EMAILSis set to 3. Consider storing this in a configuration or environment variable if it’s likely to change, or if it varies by user or environment.
75-96: Clarify no immediate return scenario.After getting a valid
patternResultfromaiDetectRecurringPattern, the function proceeds tosaveToDb. Consider capturing potential exceptions from the pattern detection separately to provide more detailed error messages in case of partial successes.
118-124: Implement the TODO.
saveToDbis currently empty, so results are never actually stored. This could cause data loss or undefined behavior for future queries. Complete the implementation to persist pattern detection outcomes.
126-153: Avoid returning large objects.
getUserWithRulesreturns user plus all rules in a single query. For performance, consider pagination or limiting to the relevant rule subset if the user has numerous rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/web/app/api/ai/pattern-match/route.ts(1 hunks)apps/web/package.json(0 hunks)apps/web/utils/actions/clean.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/package.json
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/app/api/ai/pattern-match/route.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
aiDetectRecurringPattern(23-136)DetectPatternResult(16-16)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/date.ts (1)
internalDateToDate(46-53)
🔇 Additional comments (6)
apps/web/utils/actions/clean.ts (1)
3-3: Confirm the new asynchronous handling mechanism.Replacing
waitUntilwithafterchanges how background tasks are handled, which may affect reliability of cleanup tasks if the runtime environment has different constraints. Ensure this behavior meets requirements (e.g., task completion guarantees) and doesn't lead to premature termination.apps/web/app/api/ai/pattern-match/route.ts (5)
20-20: Consider using serverless function timeouts for fail-safe.
maxDuration = 60;might limit processing to 60 seconds, but confirm if the hosting environment enforces a stricter limit. Including a fail-safe mechanism (such as a timeout in the background task) helps avoid indefinite stalls or partial operations.
24-28: Validate defaults at runtime.
patternMatchSchemadefaultsmaxResultsto 5. Confirm that users can override this value properly, and that the final usage (line 39) respects the default if not set in the request body.
32-74: Validate null user scenario first.The code cleanly checks for a valid internal API key at lines 33-36, then parses JSON. However, verifying user existence depends on
getUserWithRules(). If there's potential for the user to become invalid or row to be removed mid-request, add further checks immediately after line 32 to handle partial states.
112-116: Confirm background process completion.The route returns
{ processing: true }immediately, usingafter(process(request)). If the environment quickly terminates the underlying runtime, ensure the background process is guaranteed to run to completion (similar to previous usage ofwaitUntil).
158-199: Gracefully handle partial message fetch failures.When fetching messages, any error from
getMessage()is caught and logged, but the remaining messages attempt to proceed. Confirm whether partial data is acceptable or if there's a scenario requiring a more robust retry or fallback mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/app/api/ai/pattern-match/route.ts (1)
155-157: Refactor to unify repeated logic.
🧹 Nitpick comments (6)
apps/web/utils/actions/rule.ts (1)
461-473: Add error handling for the background fetch request.Currently, the request to
/api/reply-tracker/process-previoushas no mechanism to handle network failures or unexpected response codes, which can lead to silent failures. Consider implementing additional logic (e.g., try/catch or response status checks) to improve debuggability and resilience.apps/web/app/api/google/webhook/process-history-item.ts (1)
207-216: Consider handling potential fetch failures.The
checkPatternMatchfunction performs an awaited fetch call but doesn't handle timeouts, network errors, or non-okay statuses. Adding a try/catch block or checking response codes can help identify issues in production.apps/web/app/api/ai/pattern-match/route.ts (4)
29-31: Make constants configurable.
THRESHOLD_EMAILSandMAX_RESULTSare hardcoded. Exposing them in configuration or environment variables would allow more flexible deployments.
112-116: Good usage of background invocation.Deferring the main work via
after(process(request))helps keep the endpoint responsive. Still, watch out for concurrency if multiple background tasks run simultaneously for the same user.
118-125: Placeholder for database logic.A TODO is present to implement the database save operation. Let me know if you need support or a draft solution for persisting pattern-match results.
158-199: Consistent error handling strategy.Logging and returning an empty array avoids unhandled exceptions. However, distinguishing specific error types (network failures vs. truly empty results) would help with debugging and reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/.env.example(3 hunks)apps/web/app/api/ai/pattern-match/route.ts(1 hunks)apps/web/app/api/google/webhook/process-history-item.ts(2 hunks)apps/web/env.ts(1 hunks)apps/web/utils/actions/rule.ts(1 hunks)apps/web/utils/upstash/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/web/utils/upstash/index.ts
🧰 Additional context used
🧬 Code Definitions (3)
apps/web/utils/actions/rule.ts (2)
apps/web/env.ts (1)
env(5-227)apps/web/app/api/reply-tracker/process-previous/route.ts (1)
ProcessPreviousBody(16-16)
apps/web/app/api/google/webhook/process-history-item.ts (2)
apps/web/app/api/ai/pattern-match/route.ts (1)
PatternMatchBody(27-27)apps/web/env.ts (1)
env(5-227)
apps/web/app/api/ai/pattern-match/route.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
aiDetectRecurringPattern(23-136)DetectPatternResult(16-16)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/date.ts (1)
internalDateToDate(46-53)
🔇 Additional comments (19)
apps/web/env.ts (1)
67-67: Ensure environment variable is defined in all deployments.Now that
INTERNAL_API_KEYis a mandatory string, any missing variable in the environment will cause a validation failure. Please confirm that all your environments (development, staging, production) define this variable to prevent deployment failures.apps/web/app/api/google/webhook/process-history-item.ts (1)
19-22: New imports are appropriate.These type and environment-related imports are consistent with the rest of the codebase. No concerns here.
apps/web/app/api/ai/pattern-match/route.ts (5)
1-2: No further commentary needed.
3-20: Imports and logger initialization look consistent.All required modules for logging, request handling, and data validation are accounted for.
23-27: Validation schema is well-defined.Using
z.string().email()forsenderhelps ensure correct input. Requiring auserIdis also appropriate.
32-110: Balanced approach to processing the request in the background.The
processfunction:
- Validates the internal API key,
- Locates the user/Gmail account,
- Fetches email messages,
- Invokes AI detection logic,
- And saves results to the database.
As a small improvement, consider adding extra logging or partial error-handling for intermediate steps (e.g., if message retrieval returns fewer results than expected).
126-154: Efficient user lookup.Fetching the user along with their rules in one query minimizes multiple round-trips. Returning partial user info also helps keep the query lean.
apps/web/.env.example (12)
7-7: New Gmail Configuration AddedThe Gmail configuration section now appears at line 7. It introduces the necessary environment variables (
GOOGLE_CLIENT_ID,GOOGLE_CLIENT_SECRET,GOOGLE_ENCRYPT_SECRET, andGOOGLE_ENCRYPT_SALT) for Gmail integration. Ensure these values are populated appropriately per environment.
13-15: Google Pub/Sub Configuration IntroducedThe new Pub/Sub variables (
GOOGLE_PUBSUB_TOPIC_NAMEandGOOGLE_PUBSUB_VERIFICATION_TOKEN) have been added. The topic value is set to a placeholder (projects/abc/topics/xyz); please update this to your actual topic in production. Also, verify that the secret generation instructions help maintain security.
18-22: LLM API Key Selection Guidance AddedA guidance comment has been inserted (starting at line 19) to instruct users to set at least one of the provided API keys (e.g.,
ANTHROPIC_API_KEY,OPENROUTER_API_KEY, orOPENAI_API_KEY). This addition clarifies configuration expectations. Ensure the documentation elsewhere is in sync with these options.
31-37: Economy LLM and Internal API Security Configurations AddedA new configuration block (lines 31–37) introduces settings for cost-efficient LLM usage (
ECONOMY_LLM_PROVIDERandECONOMY_LLM_MODEL) along with internal API security variables (INTERNAL_API_KEYandAPI_KEY_SALT). Please verify that:
- These variables are used securely in the application.
- The example values or generation instructions encourage strong, unique secrets.
46-51: Optional Configuration Block AddedThe optional configuration block (lines 46–51) now includes:
NEXT_PUBLIC_APP_HOME_PATH: sets the default route if desired.LOG_ZOD_ERRORS: for error logging with Zod.CRON_SECRET: which may be used for securing automated tasks.Ensure these options are well documented so that users know when and how to override the defaults.
58-58: Sentry Configuration Header AddedThe header for Sentry error tracking is now added at line 58. This neatly groups the Sentry-related environment variables that follow it. No issues detected.
64-64: Axiom Logging Configuration Header AddedAt line 64, a new header for Axiom logging appears. This addition organizes the configuration for Axiom, using
NEXT_PUBLIC_AXIOM_DATASETandNEXT_PUBLIC_AXIOM_TOKENlater in the file.
68-74: PostHog Analytics Configuration AddedA block for PostHog analytics has been inserted (lines 68–74) including variables such as:
NEXT_PUBLIC_POSTHOG_KEYNEXT_PUBLIC_POSTHOG_HERO_ABNEXT_PUBLIC_POSTHOG_ONBOARDING_SURVEY_IDPOSTHOG_API_SECRETPOSTHOG_PROJECT_IDMake sure that these variables are updated with valid keys in each deployment to enable correct analytics tracking.
75-78: Marketing Emails Configuration Variables IntroducedThe Marketing Emails section (lines 75–78) adds variables like
RESEND_API_KEYandLOOPS_API_SECRET. Confirm that the integration for marketing emails utilizes these keys appropriately and that any documentation is adjusted accordingly.
79-81: Crisp Support Chat Configuration AddedAt lines 79–81, the configuration for Crisp support chat is added with
NEXT_PUBLIC_CRISP_WEBSITE_ID. This sets up the initial integration for live chat. Ensure the value is provided in production to enable the feature.
82-84: Sanity Blog Configuration AddedThe Sanity configuration for the blog now appears (lines 82–84) with
NEXT_PUBLIC_SANITY_PROJECT_IDandNEXT_PUBLIC_SANITY_DATASET(set to"production"). Verify that"production"is the intended dataset and that the project ID is correctly documented.
86-86: Payments Section Header AddedA header for the Payments configuration section is introduced at line 86. Although it does not change any functional values itself, it improves readability and organization of the file. Ensure consistency with similar sections.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
36-38: Handle empty emails array to avoid out-of-bounds error.Referencing
emails[0]without first checking foremails.length === 0can cause a runtime error if the array is empty.
37-38: Validate that all emails are from the same sender.The comment states that “All emails should be from the same sender,” yet there's no check that each email shares the same sender. Consider verifying this condition to prevent mismatched or inconsistent pattern detection.
apps/web/app/api/ai/pattern-match/route.ts (1)
192-194: Unify sender-based retrieval logic with existing code.A TODO comment already mentions this. Consider extracting shared logic into a reusable utility to keep the code DRY.
🧹 Nitpick comments (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
12-15: Leverage the “explanation” field for better transparency.The schema defines an “explanation” field, but it isn’t used later in the code. Consider logging or storing it to facilitate more detailed debugging and insights into the AI’s reasoning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/ai/pattern-match/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
apps/web/app/api/ai/pattern-match/route.ts (4)
apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(27-151)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/date.ts (1)
internalDateToDate(46-53)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/app/api/google/webhook/process-history-item.ts (1)
129-135: Consider adding retry or follow-up logic for failed background calls.The call to
analyzeSenderPatternis made asynchronously viaafter(). If an error occurs, the main request won't be aware. If you want guaranteed or retried behavior, consider scheduling a job in a queue instead, or re-attempting when feasible.apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
17-21: Make threshold and max results easily adjustable.
THRESHOLD_EMAILSandMAX_RESULTSare set as constants. If these criteria change often or differ by user, consider placing them in configuration or environment variables.
112-116: Beware of error handling in background tasks withafter().Since
withErrorhandles errors in the main thread, any errors thrown in the background function will only be logged but won't produce a normal HTTP response. If you need to surface background errors, you may want to implement event logs, user notifications, or a retry mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/app/api/google/webhook/process-history-item.ts(4 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/app/api/google/webhook/process-history-item.ts (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
AnalyzeSenderPatternBody(28-28)apps/web/env.ts (1)
env(5-227)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (5)
apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(27-151)apps/web/utils/middleware.ts (1)
withError(16-70)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/date.ts (1)
internalDateToDate(46-53)
🔇 Additional comments (8)
apps/web/app/api/google/webhook/process-history-item.ts (3)
20-22: Imports appear consistent.It's good to see that environment and internal API constants are imported directly, ensuring a centralized approach to configuration. No immediate concerns here.
215-244: Evaluate the need for a fallback mechanism after logging errors.This function correctly logs failures, but the system might ignore persistent errors. If repeated failures need escalation or alerting, consider hooking in an alerting mechanism or fallback. Otherwise, this is a clean approach for a background task.
1-1:❓ Verification inconclusive
Check the availability and stability of the
afterAPI.The
afterfunction from"next/server"is not widely documented in some Next.js versions. Verify support for this method or confirm that the environment is appropriate for background job scheduling.
🏁 Script executed:
#!/bin/bash # Description: Check if the "after" API is documented/supported in Next.js for the used version. # Attempt to find references to "after" within the node_modules fd "next" node_modules --exec rg "after"Length of output: 38
Reassess the Stability and Documentation of the
afterAPI
Our recent automated search (usingrgwithin node_modules) did not return any evidence confirming documentation or widespread usage of theafterAPI in the current Next.js version. Because automated checks proved inconclusive, please manually verify one of the following:
- Confirm via the official Next.js documentation or release notes whether the
afterfunction is now a supported/stable feature.- Test the API in a development environment to ensure it behaves as expected for background job scheduling.
apps/web/app/api/ai/analyze-sender-pattern/route.ts (5)
1-2: Dependency imports look appropriate.All newly introduced imports from Next.js and internal modules are consistent with the rest of the codebase. No issues here.
30-110: Ensure the “process” logic is well-tested.This function performs key checks, fetches user, Gmail account, and messages, then performs AI detection. Add or confirm robust unit tests or integration tests, ensuring each part (API key check, user retrieval, message fetch, AI detection, DB update) is validated.
118-161: Upsert logic is clean.The
saveToDbfunction properly usesupsertto avoid duplicates. This is efficient and maintains a single record per (groupId, FROM) pair. Well done.
163-191: User retrieval approach is straightforward.Filtering on
enabled: trueand loaded instructions is well-structured. No immediate concerns; just ensure that large user queries are efficient if the user has many rules.
192-236: Graceful handling of Gmail query failures.
getMessagesFromSenderlogs errors and returns an empty array upon failure, avoiding exceptions in the parent logic. This is a good fallback to keep the system robust.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
31-34:⚠️ Potential issueAdd array emptiness check and sender validation.
The function accesses
emails[0]without first verifying the array isn't empty, which could cause runtime errors. Additionally, since the function assumes all emails are from the same sender, validation should be added to ensure this is true.export async function aiDetectRecurringPattern({ emails, user, rules, }: { emails: EmailForLLM[]; user: UserEmailWithAI; rules: { name: string; instructions: string; }[]; }): Promise<DetectPatternResult | null> { // Extract the sender email from the first email // All emails should be from the same sender + if (!emails || emails.length === 0) { + return null; + } const senderEmail = emails[0].from; if (!senderEmail) return null; + + // Validate that all emails are from the same sender + const allSameFrom = emails.every(email => email.from === senderEmail); + if (!allSameFrom) { + logger.warn("Emails from different senders provided to aiDetectRecurringPattern"); + return null; + }
🧹 Nitpick comments (6)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (3)
19-21: Externalize threshold and max results configuration.The
THRESHOLD_EMAILS = 3andMAX_RESULTS = 20constants are currently hardcoded. Consider externalizing them into an environment variable or configuration file to allow for flexible adjustments without code changes.-const THRESHOLD_EMAILS = 3; -const MAX_RESULTS = 20; +const THRESHOLD_EMAILS = parseInt(process.env.THRESHOLD_EMAILS || "3", 10); +const MAX_RESULTS = parseInt(process.env.MAX_RESULTS || "20", 10);
112-116: Assess background processing for potential concurrency issues.Using
after(() => process(request))returns a response immediately but defers the actual execution. Ensure that existing concurrency or rate-limiting logic is robust enough to handle potentially large volumes of background tasks, especially if multiple requests arrive in quick succession.
192-194: Use consistent, professional language in code comments.The comment “// TODO: fix this nonsense up” may sound informal. Consider rewording to maintain a professional tone, especially in a public codebase.
-// TODO: fix this nonsense up. we have similar functions in other files +// TODO: Refactor this function. Similar functions exist in other files.apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (3)
36-96: Well-structured prompts with clear instructions.The system and user prompts are comprehensive and well-formatted. The XML-like structure helps organize information for the AI assistant, and limiting email content to 1000 characters helps manage token usage.
Consider extracting the system prompt to a separate template constant at the top of the file for better readability and maintainability, especially since it's quite lengthy.
110-131: Improve error handling and document commented code.The error handling is basic but functional. Consider adding more specific error handling for different types of errors (network issues, API limits, etc.). Also, add a comment explaining why the Braintrust dataset code is commented out and under what conditions it would be used.
try { const aiResponse = await chatCompletionObject({ userAi: user, system, prompt, schema, userEmail: user.email || "", usageLabel: "Detect recurring pattern", }); logger.trace("Response", aiResponse.object); // TODO: Uncomment and configure Braintrust integration when ready for data collection // braintrust.insertToDataset({ // id: emails[0].id, // input: { // senderEmail, // emailCount: emails.length, // sampleEmails: emails.map((email) => ({ // from: email.from, // subject: email.subject, // })), // rules: rules.map((rule) => rule.name), // }, // expected: aiResponse.object.matchedRule, // }); return aiResponse.object; } catch (error) { + // Handle different types of errors appropriately + if (error instanceof Error) { + logger.error("Error detecting recurring pattern", { + errorMessage: error.message, + errorName: error.name, + stack: process.env.NODE_ENV !== 'production' ? error.stack : undefined + }); + } else { + logger.error("Unknown error detecting recurring pattern", { error }); + } - logger.error("Error detecting recurring pattern", { error }); return null; }
18-131: Solid implementation with good attention to prompt engineering.The
aiDetectRecurringPatternfunction is well-designed for analyzing email patterns from specific senders. The prompt engineering is particularly strong, with clear instructions and examples that should guide the AI to make accurate determinations. The inclusion of a confidence threshold (>80%) is a good approach to ensure reliability.There are a few areas for improvement:
- Input validation should be strengthened as noted above
- Error handling could be more specific
- Consider adding unit tests for this function to ensure it behaves as expected with various inputs
The schema validation using Zod is a strong point that ensures type safety and consistent responses.
Would you like me to help with creating unit tests for this function? I could draft some test cases that cover different scenarios like empty arrays, mixed senders, and various response patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
🔇 Additional comments (5)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
72-88: Be cautious about passing user API keys into AI functions.You’re passing
user.aiApiKeytoaiDetectRecurringPattern. Verify that the key is never exposed in logs or error messages. If you store logs in an external system, consider masking or avoiding logging sensitive fields.
200-210: Handle edge cases for empty or large email queries.Currently, if the sender query returns no messages, you return an empty array. Large numbers of messages or partial fetches might require more robust pagination and error handling. Consider checking for rate-limit responses or partial fetch failures.
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (3)
1-8: Good use of proper imports and logging setup.The imports are well organized, and creating a scoped logger is a good practice for debugging and tracing issues specific to this feature.
12-16: Well-structured schema definition with type export.Good practice using Zod for response validation and exporting the inferred type to maintain type safety throughout the application.
98-109: Good logging practices and proper API call setup.Trace-level logging of inputs and outputs provides valuable debugging information without cluttering production logs. The API call is well-structured with all necessary parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
46-46: Handle multiple or missing accounts more robustly.
This code always picks the first account (user.accounts[0]). If users have multiple Gmail accounts or none, this logic may fail unpredictably.
🧹 Nitpick comments (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
16-17:maxDurationappears unused.
It looks like you never reference themaxDurationconstant after exporting it. Consider removing it if it’s no longer needed, or make use of it where intended.-export const maxDuration = 60;
29-33: Remove unnecessaryawaitcall and return proper status code.
Currently,headers()is synchronous, so awaiting it makes no difference. Additionally, consider returning a 401 for unauthorized access rather than a 200 OK with an error payload.- if (!isValidInternalApiKey(await headers())) { + if (!isValidInternalApiKey(headers())) { logger.error("Invalid API key"); - return NextResponse.json({ error: "Invalid API key" }); + return NextResponse.json({ error: "Invalid API key" }, { status: 401 }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (6)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/get-email-from-message.ts (1)
getEmailForLLM(5-18)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(18-131)apps/web/utils/middleware.ts (1)
withError(16-70)apps/web/utils/gmail/thread.ts (2)
getThreads(25-46)getThreadMessages(164-173)
🔇 Additional comments (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
210-247:❓ Verification inconclusive
Clarify skipping logic if a single thread is a conversation.
You return an empty array the moment you find any single thread with multiple senders, even if other threads may not be conversations. This might exclude perfectly valid threads from analysis. Confirm if the intended behavior is to skip all threads at the first conversation or skip only that one.
🏁 Script executed:
#!/bin/bash # Description: Check if there are scenarios where multiple threads are returned # but only some of them are conversations. If so, confirm whether the correct approach # is to skip all or skip only the identified conversation thread. # Gathering usage references to see how these threads are handled. rg -A 5 "getThreadsFromSender"Length of output: 4109
Clarify the Early Return Behavior on Conversation Threads
It looks like the current implementation in
apps/web/app/api/ai/analyze-sender-pattern/route.tsreturns an empty array as soon as any thread is recognized as a conversation (i.e., when a thread has messages from other senders). This means that if even one conversation exists among multiple retrieved threads, all threads are skipped—even if some threads might have been valid for analysis.
- Please confirm whether the intended behavior is to bypass analysis for the entire sender on finding any conversation, or if only the identified conversation thread should be excluded while still processing the rest.
- Note that similar functions (like
getThreadsFromSenderWithSubject) are used elsewhere in the codebase. It would be helpful to verify if they’re expected to behave differently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/utils/categorize/senders/categorize.ts (1)
114-118: Consistent email handling in database operationsUsing the extracted email in both the
whereclause andcreateobject ensures data consistency in the database. This is a good improvement.However, consider adding error handling for cases where
extractEmailAddressmight return an empty string:const email = extractEmailAddress(sender); + if (!email) { + throw new Error(`Invalid email address: ${sender}`); + } await prisma.newsletter.upsert({ where: { email_userId: { email, userId } }, update: { categoryId }, create: { email, userId, categoryId, }, });apps/web/app/api/google/webhook/process-history-item.ts (1)
216-245: Well-structured sender pattern analysis implementation with good error handling.The implementation correctly:
- Uses the internal API key for security
- Properly handles and logs errors without affecting the main process
- Provides detailed context in error logs
Consider adding a timeout to the fetch request to prevent long-running operations.
const response = await fetch( `${env.NEXT_PUBLIC_BASE_URL}/api/ai/analyze-sender-pattern`, { method: "POST", body: JSON.stringify(body), headers: { "Content-Type": "application/json", [INTERNAL_API_KEY_HEADER]: env.INTERNAL_API_KEY, }, + signal: AbortSignal.timeout(30000), // 30s timeout }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/test.yml(1 hunks)apps/web/app/api/google/webhook/block-unsubscribed-emails.ts(2 hunks)apps/web/app/api/google/webhook/process-history-item.test.ts(1 hunks)apps/web/app/api/google/webhook/process-history-item.ts(5 hunks)apps/web/app/api/user/categorize/senders/uncategorized/get-uncategorized-senders.ts(2 hunks)apps/web/app/api/user/stats/newsletters/route.ts(2 hunks)apps/web/utils/actions/categorize.ts(2 hunks)apps/web/utils/actions/unsubscriber.ts(1 hunks)apps/web/utils/ai/choose-rule/match-rules.ts(2 hunks)apps/web/utils/assistant/process-assistant-email.ts(1 hunks)apps/web/utils/categorize/senders/categorize.ts(2 hunks)apps/web/utils/email.test.ts(1 hunks)apps/web/utils/sender.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (10)
apps/web/utils/email.test.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/utils/categorize/senders/categorize.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/utils/assistant/process-assistant-email.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/app/api/google/webhook/block-unsubscribed-emails.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/app/api/user/categorize/senders/uncategorized/get-uncategorized-senders.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/utils/ai/choose-rule/match-rules.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/app/api/user/stats/newsletters/route.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/utils/actions/unsubscriber.ts (3)
apps/web/utils/actions/middleware.ts (1)
withActionInstrumentation(30-144)apps/web/utils/actions/unsubscriber.validation.ts (2)
SetNewsletterStatusBody(8-8)setNewsletterStatusBody(4-7)apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
apps/web/app/api/google/webhook/process-history-item.ts (4)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
AnalyzeSenderPatternBody(27-27)apps/web/env.ts (1)
env(5-227)apps/web/app/api/google/webhook/logger.ts (1)
logger(3-3)
apps/web/utils/actions/categorize.ts (1)
apps/web/utils/email.ts (1)
extractEmailAddress(16-34)
🔇 Additional comments (25)
.github/workflows/test.yml (1)
53-53: Environment Variable Addition:INTERNAL_API_KEYThe new entry at line 53 adds
INTERNAL_API_KEYwith the value"secret"to the environment in the "Run tests" step. This aligns with the recent changes indicating that this variable is now mandatory. Ensure that this placeholder value is acceptable for test runs and that the actual secret is securely managed via GitHub Secrets or a similar secret management system in production environments.apps/web/utils/email.test.ts (1)
42-44: Good addition to the test suite.This new test case verifies that
extractEmailAddresscorrectly handles simple email formats without angle brackets or additional formatting, which is a common use case. The test improves coverage and ensures the function works as expected with basic inputs.apps/web/utils/sender.ts (1)
11-11: Good defensive programming.Adding an early return when
apps/web/app/api/google/webhook/block-unsubscribed-emails.ts (2)
10-10: Good import addition.Adding the import for
extractEmailAddressis necessary for the new email extraction functionality.
25-29: Improves email handling robustness.Using
extractEmailAddressenhances the function's ability to handle complex email formats like "Name email@example.com" by consistently extracting just the email address portion. This standardizes email processing across the application and improves query accuracy.apps/web/utils/actions/categorize.ts (2)
26-26: Good import addition.Adding the import for
extractEmailAddressis necessary for the improved email handling.
56-58: Enhances email extraction logic.By applying
extractEmailAddresstosender.from, the code now consistently extracts standardized email addresses from potentially complex formats. This ensures that theallSendersarray contains properly formatted email addresses, which improves the accuracy of subsequent database queries and filtering operations.apps/web/app/api/user/stats/newsletters/route.ts (2)
13-13: Good addition of utility function importAdding the
extractEmailAddressutility function import improves email handling by standardizing how email addresses are extracted across the application.
85-93: Improved email address extractionExcellent refactoring to use the
extractEmailAddressutility function. This ensures consistent handling of email addresses by extracting just the email portion from strings that might contain display names (e.g., "John Doe john@example.com"). This will improve accuracy in newsletter tracking and filtering.apps/web/app/api/user/categorize/senders/uncategorized/get-uncategorized-senders.ts (2)
1-1: Good addition of utility function importAdding the
extractEmailAddressutility function import improves email handling by standardizing how email addresses are extracted across the application.
25-25: Improved email address extractionThe change to use
extractEmailAddresson sender.from values ensures proper handling of email addresses by extracting only the actual email address from strings that might contain both display names and emails (e.g., "John Doe john@example.com"). This will make the uncategorized sender detection more accurate and consistent.apps/web/utils/ai/choose-rule/match-rules.ts (2)
27-27: Good addition of utility function importAdding the
extractEmailAddressutility function import improves email handling by standardizing how email addresses are extracted across the application.
62-65: Improved email address extraction in database queryUsing
extractEmailAddressonmessage.headers.fromensures that only the proper email address is used in the database query. This improves consistency and accuracy when looking up sender information, especially when the "from" field contains both name and email (e.g., "John Doe john@example.com").apps/web/utils/assistant/process-assistant-email.ts (1)
158-158: Improved email address extraction for database queryUsing
extractEmailAddressonoriginalMessage.headers.fromensures that only the proper email address is used in the database query. This maintains consistency with the email handling approach used across the application and helps prevent potential mismatches when searching for newsletter records.apps/web/utils/categorize/senders/categorize.ts (2)
13-13: Good addition of theextractEmailAddressutilityAdding this import enhances email handling by standardizing the extraction of email addresses from various formats.
112-112: Improve email handling with consistent extractionGood implementation of the
extractEmailAddressfunction to standardize email format before database operations. This ensures that emails with display names (e.g., "John Doe john@example.com") are properly handled by extracting just the email address.apps/web/utils/actions/unsubscriber.ts (3)
6-10: Good implementation of validation with Zod schemaAdding proper validation using a Zod schema is a significant improvement for type safety and input validation. The import of
extractEmailAddressfurther enhances email handling consistency.
14-25: Improved input validation and error handlingThe changes significantly enhance the function's robustness by:
- Using proper validation with
safeParse- Providing clear error messages on validation failure
- Extracting email address consistently with
extractEmailAddress- Using user ID instead of email for authentication check
This is a meaningful improvement to the code quality and security.
28-36: Consistent database operations with extracted emailUsing the properly extracted email in the database operations ensures data consistency. The changes to the
createandupdateoperations align well with the validation improvements.apps/web/app/api/google/webhook/process-history-item.test.ts (1)
19-21: Adding theaftermock is necessary for testing the new async behavior.The mock implementation appropriately executes the callback immediately, which ensures any side effects are properly tested rather than being deferred to after the test completes.
apps/web/app/api/google/webhook/process-history-item.ts (5)
1-1: Adding import forafterfrom next/server is appropriate.This import enables the implementation of non-blocking operations in the webhook handler, which is critical for performance when processing emails.
20-23: Good addition of necessary imports.These imports provide the required types and utilities for the new sender pattern analysis functionality.
130-135: Well-implemented async pattern analysis.Using
after()for sender pattern analysis is a good approach as it:
- Doesn't block the main email processing flow
- Ensures pattern analysis happens even if the main flow returns early
- Separates concerns between immediate email processing and background analysis
169-169: Improved email extraction with dedicated utility.Replacing direct access to
message.headers.fromwith theextractEmailAddressutility improves robustness by properly handling complex email formats like "Name email@example.com".
216-245:✅ Verification successful
Verify the availability of the analyze-sender-pattern endpoint.
Make sure the API endpoint is correctly implemented and available in all environments.
🏁 Script executed:
#!/bin/bash # Check if the analyze-sender-pattern endpoint exists and is properly implemented rg -A 5 "analyze-sender-pattern/route.ts" --type tsLength of output: 51
I've generated another search command to look for files related to the endpoint, since the initial command returned no output. Please run the following shell script to search for any file names or references containing "analyze-sender-pattern" to help verify that the endpoint is implemented:
Once you run this command, please review the output manually. If no related files are found, it’s likely the endpoint isn’t implemented—so please verify manually if it should exist or if the logic is handled elsewhere.
Rewritten Review Comment:
Action: Verify the availability of the analyze-sender-pattern endpoint
The automated search for the endpoint’s route file didn’t yield any results. Please double-check manually in your codebase (e.g. under an
/api/aidirectory or similar) to ensure that the analyze-sender-pattern endpoint is correctly implemented and available in all environments. If it is implemented in a different location or under a different name, kindly update the API call accordingly.
🏁 Script executed:
#!/bin/bash # Search for any TypeScript files referencing "analyze-sender-pattern" find . -type f -name "*.ts" | grep -i "analyze-sender-pattern"Length of output: 238
Endpoint Verified: analyze-sender-pattern Endpoint Exists
The analyze-sender-pattern endpoint was located at:
•apps/web/app/api/ai/analyze-sender-pattern/route.tsThis confirms that the API endpoint is implemented and available. Please ensure that all environment configurations (such as
NEXT_PUBLIC_BASE_URLand internal API keys) are correctly set across environments to consistently support API calls.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
29-157: Handle multiple Gmail accounts in the user object.On line 66, you're always selecting the first account from
user.accounts[0]without verifying it's the appropriate Gmail account or handling cases where the user has multiple accounts.This issue was previously identified in a review. Please update the implementation to handle multiple Gmail accounts by either explicitly checking for the right account type or providing a clear fallback mechanism when multiple accounts exist.
🧹 Nitpick comments (5)
apps/web/prisma/schema.prisma (1)
337-340: New Newsletter Pattern Analysis FieldsThe addition of the
patternAnalyzedfield (with a default value offalse) and thelastAnalyzedAtfield to theNewslettermodel is clear and concise. These fields expand the schema to support tracking the analysis of learned patterns in newsletters. A few points to consider:
- If these fields are expected to be queried frequently, adding an index (or including them in an existing composite index) might improve performance.
- Ensure that your migration scripts and any related business logic (for example, within the
aiDetectRecurringPatternfunctionality) incorporate these new fields.Overall, the changes look good and align with the intended improvements.
apps/web/app/api/ai/analyze-sender-pattern/route.ts (4)
217-226: Reconsider completely rejecting conversation threads.The current implementation abandons pattern detection entirely if any thread contains messages from different senders. This might be overly strict, as many senders (like e-commerce sites) send both one-way communications and engage in customer service conversations.
Consider a more nuanced approach that filters out conversation threads but continues with pattern detection on one-way threads:
- // If we found a conversation thread, skip this sender entirely - if (hasOtherSenders) { - logger.info("Skipping sender pattern detection - conversation detected", { - from, - }); - return []; - } + // If we found a conversation thread, skip just this thread + if (hasOtherSenders) { + logger.info("Skipping conversation thread for pattern detection", { + from, + threadId: thread.id, + }); + continue; + }
282-309: Improve readability and maintainability of user selection.The
getUserWithRulesfunction selects many fields and has nested selections for accounts and rules. Consider decomposing this into smaller, more maintainable pieces or using a type-safe approach.Consider using a Prisma type for user selection or creating a dedicated type:
type UserWithRules = Prisma.UserGetPayload<{ select: { id: true; email: true; about: true; aiProvider: true; aiModel: true; aiApiKey: true; accounts: { take: 1; select: { access_token: true; refresh_token: true; }; }; rules: { where: { enabled: true; instructions: { not: null } }; select: { id: true; name: true; instructions: true; }; }; }; }>;Then reuse this type for better maintainability and consistency.
113-113: Add type safety for message mapping.When mapping messages to the
EmailForLLMformat (line 113), there's no explicit type checking to ensure the structure matches whatgetEmailForLLMexpects.Add explicit typing to the
allMessagesarray to improve type safety:- const allMessages = threadsWithMessages.flatMap( - (thread) => thread.messages, - ); + const allMessages = threadsWithMessages.flatMap( + (thread) => thread.messages as ParsedMessage[], + );This will prevent potential runtime errors if the message structure changes.
96-110: Consider more informative logging for email thresholds.The current logging simply states there aren't enough emails, but it could be more informative to include the threshold requirement.
Enhance the log message to include the threshold requirement:
if (allMessages.length < THRESHOLD_EMAILS) { logger.info("Not enough emails found from this sender", { from, userId, count: allMessages.length, + required: THRESHOLD_EMAILS, }); // Don't record a check since we didn't run the AI analysis return NextResponse.json({ success: true }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/prisma/schema.prisma(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (7)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/email.ts (1)
extractEmailAddress(16-36)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/get-email-from-message.ts (1)
getEmailForLLM(5-18)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(18-131)apps/web/utils/middleware.ts (1)
withError(16-70)apps/web/utils/gmail/thread.ts (1)
getThreadMessages(164-173)
🔇 Additional comments (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
16-16: Consider whether 60 seconds is sufficient for this operation.Setting
maxDurationto 60 seconds may be limiting for processing larger email datasets. The function fetches multiple email threads, processes them, and performs AI analysis, which could potentially exceed this timeout for users with many emails from a particular sender.Consider monitoring actual execution time in production or implementing a more robust background processing solution for long-running operations.
159-163: Well-designed asynchronous processing approach.Using
after()to handle the processing in the background while immediately responding to the client is an excellent pattern for long-running operations. This prevents connection timeouts and provides a better user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
30-34:⚠️ Potential issueMissing email array validation could cause out-of-bounds error
The function directly accesses
emails[0]without first checking if theemailsarray is empty, which could cause a runtime error.Add a check before accessing the first email:
+if (!emails || emails.length === 0) { + return null; +} const senderEmail = emails[0].from; if (!senderEmail) return null;
30-34: 🛠️ Refactor suggestionConsider validating that all emails are from the same sender
The function assumes all emails are from the same sender but only checks the first email's sender. A validation check would prevent unexpected behavior when processing emails from different senders.
const senderEmail = emails[0].from; if (!senderEmail) return null; + +// Validate that all emails are from the same sender +const allSameFrom = emails.every(email => email.from === senderEmail); +if (!allSameFrom) { + logger.warn("Emails from different senders provided to aiDetectRecurringPattern"); + return null; +}apps/web/app/api/ai/analyze-sender-pattern/route.ts (3)
56-63: Potential concurrency issue when checking if sender is already analyzed.
As noted in a past comment, usingexistingCheck?.patternAnalyzedin this manner could still allow multiple simultaneous requests to proceed. If two requests arrive before the record is updated, both could inadvertently re-analyze the same sender.
72-77: Handle multiple accounts or none.
As noted previously, relying onuser.accounts[0]may fail or yield unexpected results when a user has multiple accounts or none. Provide explicit handling or a clear fallback for these scenarios.
138-149: Consider wrapping database operations in a transaction.
A transaction can help avoid race conditions or partially committed data if multiple requests arrive simultaneously or if an error occurs betweensaveToDbandsavePatternCheck.
🧹 Nitpick comments (7)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
10-10: Remove or utilize the commented Braintrust initializationThere's a commented-out Braintrust initialization. If this is not going to be used, consider removing it to keep the code clean.
112-125: Remove or utilize the commented Braintrust dataset integrationThere's a large commented-out section for Braintrust dataset integration. If this functionality won't be implemented soon, consider removing it to improve code clarity.
36-82: Consider extracting the system prompt to a separate fileThe system prompt is quite lengthy and could be extracted to a separate file or constant to improve readability and maintainability of the main function.
+// In a new file: apps/web/utils/ai/choose-rule/prompts.ts +export const RECURRING_PATTERN_SYSTEM_PROMPT = `You are an AI assistant that helps analyze if a sender's emails should consistently be matched to a specific rule. + +<instructions> +Your task is to determine if emails from a specific sender should ALWAYS be matched to the same rule. +... +</outputFormat>`; + +// In the current file +import { RECURRING_PATTERN_SYSTEM_PROMPT } from "./prompts"; + -const system = `You are an AI assistant that helps analyze if a sender's emails should consistently be matched to a specific rule. - -<instructions> -... -</outputFormat>`; +const system = RECURRING_PATTERN_SYSTEM_PROMPT;
12-16: Consider more precise Zod schema validationThe current schema validates that
matchedRuleis a string or null, but you could make it more precise by restricting it to only valid rule names.const schema = z.object({ - matchedRule: z.string().nullable(), + matchedRule: z.union([z.literal(null), z.enum(rules.map(r => r.name))]), explanation: z.string(), });This would ensure that the matched rule is always either null or one of the valid rule names passed to the function.
98-108: Consider adding timeout handling for AI requestsLong-running AI requests might timeout or take too long to process. Consider adding a timeout mechanism or retry logic to handle potential delays.
try { + // Set a timeout for AI response + const timeoutPromise = new Promise<null>((_, reject) => { + setTimeout(() => reject(new Error("AI request timeout")), 30000); // 30 seconds timeout + }); + + const aiResponsePromise = chatCompletionObject({ userAi: user, system, prompt, schema, userEmail: user.email || "", usageLabel: "Detect recurring pattern", }); + + const aiResponse = await Promise.race([aiResponsePromise, timeoutPromise]);apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
40-43: Consider a more robust background processing approach.
Usingafter(() => process(...))immediately returns a successful response without surfacing any potential errors in the background job. If reliability is critical, you might consider a queue-based approach or another mechanism to capture and handle background processing errors more predictably.
215-225: Allow partial inclusion of valid threads if some are conversations.
Currently, detecting a conversation in any single thread causes the code to skip all threads from this sender. Consider refining this logic to exclude only the conversation-based threads while still analyzing other threads that might contain one-way communications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-choose-rule.ts(5 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/ai/choose-rule/ai-choose-rule.ts
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (7)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/middleware.ts (1)
withError(16-70)apps/web/utils/email.ts (1)
extractEmailAddress(16-36)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/get-email-from-message.ts (1)
getEmailForLLM(5-18)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(18-131)apps/web/utils/gmail/thread.ts (1)
getThreadMessages(164-173)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
🔇 Additional comments (2)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
101-108: LGTM: Good use of AI completion with proper error handlingThe function correctly uses the
chatCompletionObjectwith proper parameters, error handling, and logging. The inclusion of a schema for validation ensures type safety of the response.apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
282-309: Good modular structure for user/rules retrieval.
ThegetUserWithRulesfunction cleanly retrieves only the needed fields, promoting maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
31-34: 🛠️ Refactor suggestionValidate that all emails are from the same sender.
The comment states "All emails should be from the same sender" but there's no validation to ensure this is the case.
Add validation to ensure all emails have the same sender:
// Extract the sender email from the first email // All emails should be from the same sender const senderEmail = emails[0].from; if (!senderEmail) return null; + + // Verify all emails have the same sender + const allSameSender = emails.every(email => email.from === senderEmail); + if (!allSameSender) { + logger.warn("Emails from different senders provided to aiDetectRecurringPattern", { + senderEmail, + emailCount: emails.length + }); + return null; + }
30-34:⚠️ Potential issueHandle empty emails array to prevent null reference error.
The function extracts the sender email from the first email without checking if the array is empty, which could lead to a runtime error.
Apply this diff to check if the emails array is empty before accessing its first element:
+ if (!emails || emails.length === 0) { + return null; + } const senderEmail = emails[0].from; if (!senderEmail) return null;apps/web/app/api/ai/analyze-sender-pattern/route.ts (4)
72-77:⚠️ Potential issueHandle the case of multiple or no Gmail accounts.
The code assumes
user.accounts[0]exists and is a Gmail account without validating the array length.Add proper validation before accessing the first account:
- const account = user.accounts[0]; + if (user.accounts.length === 0) { + logger.error("No accounts found for user", { userId }); + return NextResponse.json({ success: false }, { status: 404 }); + } + + const account = user.accounts[0];
138-149: 🛠️ Refactor suggestionWrap database operations in a transaction.
The code performs multiple database operations without using a transaction. If one operation succeeds but the other fails, it could leave the database in an inconsistent state.
Wrap the related database operations in a transaction:
if (patternResult?.matchedRule) { + await prisma.$transaction(async (tx) => { // Save pattern to DB (adds sender to rule's group) await saveLearnedPattern({ + prismaClient: tx, userId, from, ruleName: patternResult.matchedRule, }); + // Record the pattern analysis result + await savePatternCheck(userId, from, tx); + }); - } - - // Record the pattern analysis result - await savePatternCheck(userId, from); + } else { + // Record the pattern analysis result without a pattern match + await savePatternCheck(userId, from); + }This will require updating the function signatures of
saveLearnedPatternandsavePatternCheckto accept an optional transaction client.
56-63: 🛠️ Refactor suggestionConsider using a transaction to prevent race conditions.
There's a potential race condition between checking if a sender has been analyzed and later recording the analysis.
Implement a database transaction or a locking mechanism:
- const existingCheck = await prisma.newsletter.findUnique({ - where: { email_userId: { email: extractEmailAddress(from), userId } }, - }); - - if (existingCheck?.patternAnalyzed) { - logger.info("Sender has already been analyzed", { from, userId }); - return NextResponse.json({ success: true }); - } + // Use upsert with a conditional update to atomically check and mark as processing + const existingCheck = await prisma.newsletter.upsert({ + where: { email_userId: { email: extractEmailAddress(from), userId } }, + update: { + processing: true, + processingStartedAt: new Date() + }, + create: { + email: extractEmailAddress(from), + userId, + processing: true, + processingStartedAt: new Date() + }, + }); + + if (existingCheck.patternAnalyzed && !existingCheck.processing) { + logger.info("Sender has already been analyzed", { from, userId }); + return NextResponse.json({ success: true }); + }
256-259: 🛠️ Refactor suggestionImprove error handling when rule is not found.
If a rule is not found, the function logs an error and returns silently without informing the caller.
Return a clear error value or throw an exception:
if (!rule) { logger.error("Rule not found", { userId, ruleName }); - return; + throw new Error(`Rule '${ruleName}' not found for user ${userId}`); }
🧹 Nitpick comments (3)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
10-11: Commented-out Braintrust integration code.There's a commented-out line related to Braintrust which seems to be for dataset integration.
Is this integration planned for the future? If so, consider adding a TODO comment explaining when/why it will be activated. I can help create an issue to track this implementation if needed.
112-125: Commented-out dataset collection code.This section appears to be intended for collecting training data for machine learning purposes but is currently inactive.
If data collection is important for improving the model accuracy over time, would you like me to help implement a feature flag to conditionally enable this functionality?
apps/web/app/api/ai/analyze-sender-pattern/route.ts (1)
219-226: Reconsider early return on first conversation thread.The function returns an empty array as soon as it finds a thread with multiple senders, without checking other threads. This might be too restrictive if only one thread out of many is a conversation.
Consider filtering out conversation threads instead of skipping the sender entirely:
- // If we found a conversation thread, skip this sender entirely - if (hasOtherSenders) { - logger.info("Skipping sender pattern detection - conversation detected", { - from, - }); - return []; - } - - threadsWithMessages.push({ - threadId: thread.id, - messages, - }); + // Only include threads that aren't conversations + if (!hasOtherSenders) { + threadsWithMessages.push({ + threadId: thread.id, + messages, + }); + } else { + logger.info("Skipping conversation thread", { + from, + threadId: thread.id, + }); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts(1 hunks)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/web/app/api/ai/analyze-sender-pattern/route.ts (7)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/middleware.ts (1)
withError(16-70)apps/web/utils/email.ts (1)
extractEmailAddress(16-36)apps/web/utils/gmail/client.ts (1)
getGmailClient(28-33)apps/web/utils/get-email-from-message.ts (1)
getEmailForLLM(5-18)apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (1)
aiDetectRecurringPattern(18-131)apps/web/utils/gmail/thread.ts (1)
getThreadMessages(164-173)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (5)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/types.ts (1)
EmailForLLM(104-112)apps/web/utils/llms/types.ts (1)
UserEmailWithAI(4-5)apps/web/utils/stringify-email.ts (1)
stringifyEmail(4-16)apps/web/utils/llms/index.ts (1)
chatCompletionObject(128-132)
🔇 Additional comments (5)
apps/web/utils/ai/choose-rule/ai-detect-recurring-pattern.ts (2)
38-57: Excellent prompt engineering with clear instructions.The system prompt is well-structured with detailed instructions that clearly define when to match a rule (>80% confidence) and when to return null. The examples and clarity in the instructions should help the AI model make better decisions.
100-127: Proper error handling with logging.The function has robust error handling that catches exceptions and logs errors, ensuring the function returns null in case of failures rather than propagating errors to callers.
apps/web/app/api/ai/analyze-sender-pattern/route.ts (3)
16-20: Good use of constants for configurable thresholds.Using named constants for email thresholds and maximum results makes the code more maintainable and easier to adjust in the future.
40-43: Effective use of background processing with after().The API returns immediately while processing continues in the background, which provides a better user experience for potentially long-running operations.
107-116: Good threshold validation before running expensive AI operations.The check for a minimum number of emails before proceeding to pattern detection is an effective optimization to avoid unnecessary AI processing.
| async function savePatternCheck(userId: string, from: string) { | ||
| await prisma.newsletter.upsert({ | ||
| where: { | ||
| email_userId: { | ||
| email: from, | ||
| userId, | ||
| }, | ||
| }, | ||
| update: { | ||
| patternAnalyzed: true, | ||
| lastAnalyzedAt: new Date(), | ||
| }, | ||
| create: { | ||
| email: from, | ||
| userId, | ||
| patternAnalyzed: true, | ||
| lastAnalyzedAt: new Date(), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction support to savePatternCheck function.
Similar to the saveLearnedPattern function, this function should also accept an optional transaction client.
Update the function signature:
- async function savePatternCheck(userId: string, from: string) {
+ async function savePatternCheck(userId: string, from: string, prismaClient?: typeof prisma) {
+ const db = prismaClient || prisma;
- await prisma.newsletter.upsert({
+ await db.newsletter.upsert({
// ...📝 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.
| async function savePatternCheck(userId: string, from: string) { | |
| await prisma.newsletter.upsert({ | |
| where: { | |
| email_userId: { | |
| email: from, | |
| userId, | |
| }, | |
| }, | |
| update: { | |
| patternAnalyzed: true, | |
| lastAnalyzedAt: new Date(), | |
| }, | |
| create: { | |
| email: from, | |
| userId, | |
| patternAnalyzed: true, | |
| lastAnalyzedAt: new Date(), | |
| }, | |
| }); | |
| } | |
| async function savePatternCheck(userId: string, from: string, prismaClient?: typeof prisma) { | |
| const db = prismaClient || prisma; | |
| await db.newsletter.upsert({ | |
| where: { | |
| email_userId: { | |
| email: from, | |
| userId, | |
| }, | |
| }, | |
| update: { | |
| patternAnalyzed: true, | |
| lastAnalyzedAt: new Date(), | |
| }, | |
| create: { | |
| email: from, | |
| userId, | |
| patternAnalyzed: true, | |
| lastAnalyzedAt: new Date(), | |
| }, | |
| }); | |
| } |
elie222
changed the title
Needs more checking
Summary by CodeRabbit
New Features
Tests
Chores