Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduce functionality for handling system types in rules, enhancing the integration of Gmail context into rule matching. The UI components are updated to conditionally display badges based on the Changes
Sequence Diagram(s)sequenceDiagram
participant Msg as Incoming Message
participant RM as Rule Matcher
participant GAPI as Gmail API
participant CRH as Reply History Checker
Msg->>RM: Process message for matching
RM->>RM: Check for calendar event preset
alt Calendar Event Detected
RM->>RM: Select CALENDAR rule preset
else Non-calendar Event
RM->>GAPI: Invoke Gmail API with sender info
GAPI->>CRH: Execute reply history check
CRH-->>GAPI: Return reply status and count
GAPI-->>RM: Provide reply data
RM->>RM: Filter out TO_REPLY preset if criteria met
end
RM-->>Msg: Return matched rule result
sequenceDiagram
participant U as User
participant RF as RuleForm Component
participant GD as getSystemTypeDisplay
participant UI as UI Badge/Tooltip
U->>RF: Load rule form
RF->>GD: Get display info for system type
GD-->>RF: Return system type name and description
RF->>UI: Render system type badge with tooltip
UI-->>U: Display system type information
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/utils/actions/rule.validation.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/reply-tracker/check-sender-reply-history.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 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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: 1
🧹 Nitpick comments (2)
apps/web/utils/ai/choose-rule/match-rules.ts (2)
51-69: Potential for multi-rule handling.
The current logic immediately returns after finding a matching calendar rule. If future requirements involve applying multiple calendar rules, consider continuing evaluation rather than returning early.
308-352: Make threshold and user context configurable.
At present, the threshold of 10 messages is hard-coded. If you expect dynamic requirements or user-specific variations, consider an approach that parametrizes the threshold and integrates user context for more flexible filtering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/(app)/automation/RuleForm.tsx(5 hunks)apps/web/app/(app)/automation/rule/create/page.tsx(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/utils/actions/rule.ts(2 hunks)apps/web/utils/actions/rule.validation.ts(2 hunks)apps/web/utils/ai/choose-rule/match-rules.ts(5 hunks)apps/web/utils/ai/choose-rule/types.ts(2 hunks)apps/web/utils/condition.ts(3 hunks)apps/web/utils/config.ts(1 hunks)apps/web/utils/reply-tracker/query.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/web/app/(app)/automation/RuleForm.tsx (2)
apps/web/utils/config.ts (1)
CoreConditionType(14-14)apps/web/components/ui/badge.tsx (1)
Badge(39-39)
apps/web/utils/ai/choose-rule/types.ts (1)
apps/web/utils/config.ts (2)
ConditionType(5-11)ConditionType(13-13)
apps/web/app/(app)/automation/rule/create/page.tsx (1)
apps/web/utils/config.ts (1)
CoreConditionType(14-14)
apps/web/utils/condition.ts (1)
apps/web/utils/config.ts (3)
CoreConditionType(14-14)ConditionType(5-11)ConditionType(13-13)
🔇 Additional comments (27)
apps/web/utils/config.ts (2)
10-10: LGTM: New condition type addedThe
PRESETvalue is added toConditionTypeenum, allowing preset-based conditions throughout the application.
14-14: Well-defined type utilityThe
CoreConditionTypeutility type properly excludes "GROUP" and "PRESET" from theConditionType, focusing on the core condition types that can be directly configured. This is a good practice for type safety.apps/web/utils/actions/rule.validation.ts (2)
6-6: New import added correctlyThe
PresetTypeenum is properly imported from@prisma/client.
124-124: Validation properly implemented for new fieldThe
presetTypefield is correctly defined in the schema with appropriate validation, allowing only the specific preset types defined in the enum and marking it as optional with.nullish().apps/web/prisma/schema.prisma (2)
209-211: Good documentation and field structureThe comment explaining the purpose of the field is helpful, and the field definition is correctly structured as a nullable
PresetType.
608-611: Well-defined enum for preset typesThe
PresetTypeenum is defined with clear values that reflect their intended purpose:CALENDARfor calendar-related rules andTO_REPLYfor reply-related rules.apps/web/utils/ai/choose-rule/types.ts (3)
1-7: Required import added for type safetyThe
PresetTypeimport is correctly added to support the new preset functionality in the type system.
30-33: Well-structured type definitionThe
PresetMatchtype is correctly defined, following the same pattern as other match types with a type field that extracts the specific condition type and includes the necessarypresetTypeproperty.
35-40: Union type properly updatedThe
MatchReasonunion type has been correctly updated to include the newPresetMatchtype, ensuring type safety throughout the application when working with rule matches.apps/web/app/(app)/automation/RuleForm.tsx (6)
34-34: Added PresetType import for new preset feature.The import of
PresetTypefrom@prisma/clientis correctly added to support the new preset type functionality in the form.
36-36: Updated import to use CoreConditionType.Import was properly updated to include the
CoreConditionTypetype, which excludes "GROUP" and "PRESET" condition types as defined in the config file.
122-126: Added preset type display information.This code nicely handles the conditional display of preset information in the form, checking if the rule has a preset type before attempting to get its display information.
204-204: Updated type definition for unusedCondition.The type has been correctly changed from
Exclude<ConditionType, "GROUP">toCoreConditionTypeto maintain consistency with the updated type system.
272-277: Added UI components to display preset information.The implementation correctly uses a Badge component to display the preset name along with a tooltip explaining its purpose. This helps users understand the special behavior of preset rules.
979-996: Well-implemented preset type display function.The
getPresetTypeDisplayfunction is well-structured with a clear switch statement to handle different preset types, providing appropriate name and description for each type. The default case handles any future preset types that might be added.apps/web/app/(app)/automation/rule/create/page.tsx (2)
5-5: Updated import to use CoreConditionType.The import statement has been correctly updated to use
CoreConditionTypeinstead ofConditionType, ensuring consistency with the type system changes.
11-11: Updated type definition in searchParams.The type for the
typeproperty insearchParamshas been updated to useCoreConditionTypeinstead ofExclude<ConditionType, "GROUP">, maintaining consistency with the updated type system throughout the application.apps/web/utils/actions/rule.ts (2)
88-88: Added presetType to rule creation data.This line correctly adds the
presetTypeproperty to the data object when creating a rule, with appropriate fallback toundefinedif not provided.
163-163: Added presetType to rule update data.This line correctly adds the
presetTypeproperty to the data object when updating a rule, with appropriate fallback toundefinedif not provided.apps/web/utils/condition.ts (3)
7-7: Updated import to include CoreConditionType.Import statement has been correctly updated to include
CoreConditionTypealongsideConditionType, supporting the type system changes in the application.
94-94: Updated parameter type for getEmptyCondition.The parameter type for
getEmptyConditionhas been correctly updated fromExclude<ConditionType, "GROUP">toCoreConditionType, maintaining consistency with the type system changes.
182-183: Added PRESET case to conditionTypeToString.This addition properly handles the new
PRESETcondition type in theconditionTypeToStringfunction, ensuring it returns the correct string representation.apps/web/utils/reply-tracker/query.ts (2)
22-26: Re-examine the fallback tohasReplied: true.
WhenextractEmailAddressfails, this function defaults to{ hasReplied: true }. This may inadvertently skip a needed reply for invalid emails, so verify this aligns with your intended logic.
28-29: Default logic is sound.
Returning{ hasReplied: false, receivedCount: 0 }is straightforward and paves the way for future Gmail API integration.apps/web/utils/ai/choose-rule/match-rules.ts (3)
11-16: No concerns with the new imports.
These imported types and enums appear necessary for the new calendar and reply preset features.
29-30: Effective use of newly introduced utilities.
BothanalyzeCalendarEventandcheckSenderReplyHistoryare integrated below, enhancing the rule evaluation logic.
173-179: Placement of the TO_REPLY filter is appropriate.
Applying the filter before final AI-based matching makes logical sense. This ensures that invalid reply scenarios are excluded before further processing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/utils/ai/choose-rule/match-rules.ts (1)
35-35: Consider making the threshold configurableThe threshold for determining when to filter out TO_REPLY rules is hardcoded. Consider making this configurable per user or exposing it as a system setting.
-const TO_REPLY_RECEIVED_THRESHOLD = 10; +// Make configurable via environment or user settings +const TO_REPLY_RECEIVED_THRESHOLD = process.env.TO_REPLY_THRESHOLD + ? parseInt(process.env.TO_REPLY_THRESHOLD, 10) + : 10;apps/web/utils/reply-tracker/check-sender-reply-history.ts (1)
62-107: Export function with clear documentation and parallel processing.The
checkSenderReplyHistoryfunction:
- Has excellent JSDoc comments explaining its purpose and parameters
- Properly extracts and validates the sender email
- Efficiently runs checks in parallel using Promise.all
- Includes comprehensive error handling and logging
- Returns sensible default values in error cases
One minor suggestion:
Consider adding a timeout to the Promise.all call to prevent long-running queries:
// Run checks in parallel for efficiency const [hasReplied, receivedCount] = await Promise.all([ checkIfReplySent(gmail, cleanSenderEmail), countReceivedMessages(gmail, cleanSenderEmail, receivedThreshold), - ]); + ]).catch(error => { + logger.error("Timeout or error in parallel operations", { + error, + cleanSenderEmail + }); + return [true, 0]; // Safe defaults + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/utils/ai/choose-rule/match-rules.test.ts(16 hunks)apps/web/utils/ai/choose-rule/match-rules.ts(9 hunks)apps/web/utils/ai/choose-rule/run-rules.ts(1 hunks)apps/web/utils/reply-tracker/check-sender-reply-history.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/utils/ai/choose-rule/run-rules.ts (1)
apps/web/utils/ai/choose-rule/match-rules.ts (1)
findMatchingRule(205-216)
apps/web/utils/ai/choose-rule/match-rules.test.ts (1)
apps/web/utils/ai/choose-rule/match-rules.ts (1)
findMatchingRule(205-216)
🔇 Additional comments (12)
apps/web/utils/ai/choose-rule/match-rules.ts (5)
1-1: Added necessary imports for preset functionalityThe new imports correctly support the calendar event detection and reply history tracking features.
Also applies to: 16-17, 30-31
56-74: Well-implemented calendar event detectionThe calendar event detection and matching logic is clear and straightforward, with proper logging and early returns for improved performance.
179-185: Appropriately filtering potential matchesThe updated return value correctly applies the TO_REPLY preset filtering logic to potential matches before returning them.
209-209: Consistently updated function signaturesFunction signatures are consistently updated throughout the file to include the Gmail parameter.
Also applies to: 222-222
318-365: Robust implementation of reply history filteringThe TO_REPLY preset filtering logic is well-implemented with:
- Early returns for efficiency
- Clear error handling that doesn't block rule processing on failure
- Informative logging
- Careful consideration of edge cases
This effectively prevents showing TO_REPLY prompts for senders who have sent many messages without receiving replies.
apps/web/utils/ai/choose-rule/run-rules.ts (1)
48-48: Update to include Gmail context in rule matching logic.The parameter
gmailis now passed tofindMatchingRulewhich aligns with the new function signature inmatch-rules.ts. This enables the rule matching logic to utilize Gmail-specific functionalities like checking sender reply history for preset rules.apps/web/utils/ai/choose-rule/match-rules.test.ts (3)
2-2: Import of Gmail type added correctly.The
gmail_v1type from@googleapis/gmailis correctly imported to support the updated function signature.
24-24: Gmail mock object added appropriately.An empty Gmail mock object is created to pass to
findMatchingRulecalls throughout the test file. This allows tests to remain valid without requiring actual Gmail API interactions.
110-110: Test cases updated to include Gmail parameter.All test cases have been properly updated to include the
gmailparameter in their calls tofindMatchingRule, maintaining consistency with the updated function signature.Also applies to: 124-124, 138-138, 163-163, 182-182, 201-201, 237-237, 267-267, 298-298, 320-320, 341-341, 383-383, 423-423, 464-464
apps/web/utils/reply-tracker/check-sender-reply-history.ts (3)
1-7: Well-structured imports and logger setup.The file correctly imports necessary dependencies and establishes a scoped logger for tracking reply-related queries.
8-26: Helper function implemented with proper error handling.The
checkIfReplySentfunction correctly queries the Gmail API to check if the user has sent a reply to the specified sender. The error handling is appropriate, defaulting totrueon error which is a safer approach for TO_REPLY filtering.
28-60: Message counting function appropriately limited by threshold.The
countReceivedMessagesfunction efficiently counts messages from a sender up to the provided threshold. The query and error handling are well-implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/utils/reply-tracker/check-sender-reply-history.ts (2)
75-107: Consider adding pagination for more accurate message countingThe current implementation only counts messages up to the threshold in a single request. If there's a need to accurately count all messages (even if capped at the threshold), consider implementing pagination to retrieve all relevant messages.
Example implementation with pagination:
async function countReceivedMessages( gmail: gmail_v1.Gmail, cleanSenderEmail: string, threshold: number, ): Promise<number> { try { const query = `from:${cleanSenderEmail}`; logger.info(`Checking received message count (up to ${threshold})`, { cleanSenderEmail, threshold, }); - // Fetch up to the threshold number of message IDs. - const response = await getMessages(gmail, { - query, - maxResults: threshold, - }); - const count = response.messages?.length ?? 0; + // Implement pagination to get accurate count up to threshold + let count = 0; + let pageToken: string | undefined; + + do { + const response = await getMessages(gmail, { + query, + maxResults: Math.min(100, threshold - count), // Gmail API usually limits to 100 per page + pageToken, + }); + + count += response.messages?.length ?? 0; + pageToken = response.nextPageToken; + + // Stop if we've reached the threshold or there are no more pages + } while (pageToken && count < threshold); logger.info("Received message count check result", { cleanSenderEmail, count, }); return count; } catch (error) { logger.error("Error counting received messages", { error, cleanSenderEmail, }); return 0; // Default to 0 on error } }
16-53: Consider validating the receivedThreshold parameterThe
receivedThresholdparameter is not validated. Consider adding validation to ensure it's a positive number to prevent unexpected behavior.export async function checkSenderReplyHistory( gmail: gmail_v1.Gmail, senderEmail: string, receivedThreshold: number, ): Promise<{ hasReplied: boolean; receivedCount: number }> { + // Validate threshold is a positive number + if (!Number.isInteger(receivedThreshold) || receivedThreshold <= 0) { + logger.warn("Invalid receivedThreshold parameter", { receivedThreshold }); + receivedThreshold = 5; // Default to a reasonable value + } + const cleanSenderEmail = extractEmailAddress(senderEmail); if (!cleanSenderEmail) { logger.warn("Could not extract email from sender", { senderEmail }); // Default to assuming a reply might be needed if email is invalid return { hasReplied: true, receivedCount: 0 }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/reply-tracker/check-sender-reply-history.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/utils/reply-tracker/check-sender-reply-history.ts (3)
apps/web/utils/logger.ts (1)
createScopedLogger(17-65)apps/web/utils/email.ts (1)
extractEmailAddress(16-36)apps/web/utils/gmail/message.ts (1)
getMessages(191-209)
🔇 Additional comments (5)
apps/web/utils/reply-tracker/check-sender-reply-history.ts (5)
1-7: Good job with imports and logger setupThe imports are well-organized and the scoped logger is appropriately named for the module's context.
8-26: Default return value may lead to confusing behaviorThe function returns
{ hasReplied: true, receivedCount: 0 }when it cannot extract an email. While the comment explains this is to assume "a reply might be needed," this behavior might be counterintuitive. Consider if returning this default value aligns with your application's expected behavior.Also, note that extractEmailAddress can return an empty string for invalid inputs, which your function correctly handles.
28-53: Well-structured async processing with proper error handlingExcellent job using Promise.all to run the checks in parallel for efficiency. The error handling is comprehensive with appropriate logging that includes relevant context.
71-72: Verify default error behavior aligns with business logicDefaulting to
return trueon error might mask actual issues. While the comment explains it's "safer for TO_REPLY filtering," ensure this aligns with the expected application behavior.
55-73:Details
❓ Verification inconclusive
Consider query parameter sanitization
While you're using the cleaned email address, Gmail queries might benefit from additional sanitization to prevent potential query injection issues.
🏁 Script executed:
#!/bin/bash # Check if the cleanSenderEmail variable is properly sanitized for Gmail query # Look for other instances of Gmail query construction to see if sanitization is applied rg -A 2 -B 2 "query = " --glob "*.ts" # Check if there's any existing sanitization function for Gmail queries rg "sanitize.*query|clean.*query" --glob "*.ts"Length of output: 10307
Review: Ensure Robust Sanitization of Gmail Query Parameters
While the code uses the
cleanSenderEmailvariable when constructing the Gmail query, our search indicates that there isn’t a dedicated sanitization function (e.g., one named likesanitizeQuery) applied across similar query constructions in the codebase. Although the variable name suggests prior cleaning, it isn’t clear if that step fully escapes or validates the input to prevent query injection.
- Please verify that
cleanSenderEmailis rigorously sanitized (or consider adding an explicit sanitization/escaping mechanism) before it's interpolated into the query string.- Review upstream cleaning routines to ensure they account for all potential query injection vectors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/utils/ai/choose-rule/match-rules.ts (2)
35-35: Document the threshold value selection.The
TO_REPLY_RECEIVED_THRESHOLDconstant value of 10 appears to be an important business rule threshold. Consider adding a comment explaining why this specific value was chosen and the reasoning behind it.-const TO_REPLY_RECEIVED_THRESHOLD = 10; +/** + * Threshold for the number of emails received from a sender without reply + * before we stop suggesting TO_REPLY preset. This ensures we don't keep + * suggesting replies to senders the user consistently ignores. + */ +const TO_REPLY_RECEIVED_THRESHOLD = 10;
319-365: Enhance documentation for TO_REPLY filtering logic.The
filterToReplyPresetfunction implements sophisticated filtering logic for TO_REPLY preset rules, but the business logic could be better documented. Additionally, the function assumes the sender's email is in a specific format inmessage.headers.from.Consider adding more detailed comments explaining:
- Why we filter out TO_REPLY preset for senders with no reply history but high received count
- How to extract and validate the sender email more robustly
// Helper function to filter out TO_REPLY preset if conditions met async function filterToReplyPreset( potentialMatches: (RuleWithActionsAndCategories & { instructions: string })[], message: ParsedMessage, gmail: gmail_v1.Gmail, ): Promise<(RuleWithActionsAndCategories & { instructions: string })[]> { + // We filter out TO_REPLY preset rules when the user has received many emails + // from a sender but never replied. This indicates the user doesn't want + // to engage with this sender, so we shouldn't keep suggesting to reply. + const toReplyRuleIndex = potentialMatches.findIndex( (r) => r.presetType === PresetType.TO_REPLY, ); if (toReplyRuleIndex === -1) { return potentialMatches; // No TO_REPLY rule found } + // Extract sender email - in the future, consider using a more robust + // email extraction function that handles different formats const senderEmail = message.headers.from; if (!senderEmail) { return potentialMatches; // Cannot check history without sender email }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/utils/ai/choose-rule/match-rules.ts(9 hunks)apps/web/utils/parse/calender-event.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/web/utils/parse/calender-event.ts (1)
apps/web/utils/types.ts (1)
ParsedMessage(47-58)
🔇 Additional comments (4)
apps/web/utils/parse/calender-event.ts (1)
236-250: Great implementation of the ICS attachment detection.The
hasIcsAttachmentfunction is well-implemented with good defensive coding. It properly checks if attachments exist before attempting to iterate through them, and uses optional chaining (?.) withtoLowerCase()to handle potential undefined values safely.apps/web/utils/ai/choose-rule/match-rules.ts (3)
57-74: Good implementation of calendar event preset matching.The calendar event detection logic is well-implemented and gives priority to calendar event matching over other rules, which makes sense for this use case. The code properly logs matches for easier debugging.
179-185: Clean implementation of TO_REPLY preset filtering.The updated return statement properly filters potential matches based on the sender's reply history before returning them. This is a clean implementation that maintains the existing structure while adding new functionality.
41-51:Details
✅ Verification successful
Verify all callers of the updated functions.
The
gmailparameter has been added to multiple function signatures. Ensure that all callers of these functions throughout the codebase have been updated to include this parameter.Also applies to: 205-210, 218-223
🏁 Script executed:
#!/bin/bash # Check for possible callers of these functions that might need updates # Check for findMatchingRule usage echo "Checking for findMatchingRule usage:" rg -A 2 "findMatchingRule\(" --type=ts --type=js # Check for findMatchingRuleWithReasons usage echo "Checking for findMatchingRuleWithReasons usage:" rg -A 2 "findMatchingRuleWithReasons\(" --type=ts --type=js # Check for findPotentialMatchingRules usage echo "Checking for findPotentialMatchingRules usage:" rg -A 2 "findPotentialMatchingRules\(" --type=ts --type=jsLength of output: 5631
Call sites correctly include the new
gmailparameter.
Verification confirms that all callers of the functions—namelyfindMatchingRule,findMatchingRuleWithReasons, andfindPotentialMatchingRules—have been updated to pass thegmailparameter. Specifically, the updates in the following files have been verified:
apps/web/utils/ai/choose-rule/run-rules.tsapps/web/utils/ai/choose-rule/match-rules.test.tsapps/web/utils/ai/choose-rule/match-rules.tsNo additional changes are required.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
apps/web/utils/rule/rule.ts (3)
52-56:⚠️ Potential issueMissing systemType parameter in error recovery flow.
When recreating a rule due to a duplicate name error, the systemType parameter is not passed to createRule, which will cause the system type to be lost.
Apply this fix to ensure system type is preserved:
const rule = await createRule({ result: { ...result, name: `${result.name} - ${Date.now()}` }, userId, categoryIds, + systemType, });
153-158:⚠️ Potential issuesystemType parameter missing from updateRule function.
The updateRule function doesn't have a systemType parameter, and the function doesn't update the systemType field in the database. This means system types cannot be updated once set.
Consider adding systemType support to the updateRule function:
async function updateRule( ruleId: string, result: CreateOrUpdateRuleSchemaWithCategories, userId: string, categoryIds?: string[] | null, + systemType?: SystemType | null, ) { return prisma.rule.update({ where: { id: ruleId }, data: { name: result.name, userId, + systemType, // NOTE: this is safe for now as `Action` doesn't have relations // ...rest of the function
84-88:⚠️ Potential issuesystemType not passed when creating a rule in safeUpdateRule error flow.
When handling a duplicate name error in safeUpdateRule, the systemType is not passed to createRule, which will result in the system type being lost.
This should be fixed by passing systemType to createRule:
const rule = await createRule({ result: { ...result, name: `${result.name} - ${Date.now()}` }, userId, categoryIds, + systemType, // Need to pass systemType from parent function });Note: This requires adding the systemType parameter to the safeUpdateRule function as well.
🧹 Nitpick comments (8)
apps/web/utils/ai/choose-rule/match-rules.ts (3)
35-36: Consider making TO_REPLY_RECEIVED_THRESHOLD configurableWhile defining this as a constant is good practice, consider making this value configurable via environment variables or user settings to allow flexibility without code changes.
-const TO_REPLY_RECEIVED_THRESHOLD = 10; +// Number of emails required before filtering out TO_REPLY preset rules +const TO_REPLY_RECEIVED_THRESHOLD = Number( + process.env.TO_REPLY_RECEIVED_THRESHOLD || "10" +);
319-365: Robust implementation with proper error handlingThe
filterToReplyPresetfunction is well-implemented with:
- Clear logic for identifying TO_REPLY rules
- Proper null checks for sender email
- Robust error handling that logs issues without breaking the main flow
- Informative logging of filtering decisions
One suggestion:
Consider adding a unit test specifically for this function to ensure the filtering logic works as expected with different combinations of reply history data.
344-355: Consider enhancing logging with more contextThe logging is good, but consider adding additional context like the messageId to make troubleshooting easier.
logger.info( "Filtering out TO_REPLY rule due to no prior reply and high received count", { ruleId: potentialMatches[toReplyRuleIndex].id, senderEmail, receivedCount, + messageId: message.id, }, );apps/web/app/(app)/automation/RuleForm.tsx (1)
978-982: Consider a more extensible approach for determining badge visibility.The current implementation hardcodes specific system types that should show a badge. This may require frequent updates as more system types are added.
Consider using a more maintainable approach, such as:
-function showSystemTypeBadge(systemType?: SystemType | null): boolean { - if (systemType === SystemType.TO_REPLY) return true; - if (systemType === SystemType.CALENDAR) return true; - return false; -} +const SYSTEM_TYPES_WITH_BADGES = new Set([ + SystemType.TO_REPLY, + SystemType.CALENDAR, +]); + +function showSystemTypeBadge(systemType?: SystemType | null): boolean { + return !!systemType && SYSTEM_TYPES_WITH_BADGES.has(systemType); +}apps/web/utils/reply-tracker/enable.ts (4)
101-102: Parameters added to safeCreateRule functionThe
nullparameter's purpose is not clear without additional context. Consider adding a comment to explain what this parameter represents for better code maintainability.const newRule = await safeCreateRule( { name: RuleName.ToReply, condition: { aiInstructions: defaultReplyTrackerInstructions, }, actions: [ { type: ActionType.LABEL, fields: { label: NEEDS_REPLY_LABEL_NAME, to: null, subject: null, content: null, cc: null, bcc: null, webhookUrl: null, }, }, ], }, userId, - null, + null, // Optional parameter for rule options/metadata SystemType.TO_REPLY, );
105-108: Consider updating error messageThe error message mentions "Reply Zero" which might be a legacy term if the functionality has been renamed to "Reply Tracker".
if ("error" in newRule) { - logger.error("Error enabling Reply Zero", { error: newRule.error }); - return { error: "Error enabling Reply Zero" }; + logger.error("Error enabling Reply Tracker", { error: newRule.error }); + return { error: "Error enabling Reply Tracker" }; }
113-114: Consider updating error messageSame issue with "Reply Zero" naming.
- logger.error("Error enabling Reply Zero, no rule found"); - return { error: "Error enabling Reply Zero" }; + logger.error("Error enabling Reply Tracker, no rule found"); + return { error: "Error enabling Reply Tracker" };
129-130: Consider updating error messageSame issue with "Reply Zero" naming.
- logger.error("Error enabling Reply Zero", { error: "No rule found" }); - return { error: "Error enabling Reply Zero" }; + logger.error("Error enabling Reply Tracker", { error: "No rule found" }); + return { error: "Error enabling Reply Tracker" };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/web/app/(app)/automation/RuleForm.tsx(5 hunks)apps/web/app/(app)/automation/onboarding/CategoriesSetup.tsx(1 hunks)apps/web/app/(app)/automation/onboarding/page.tsx(5 hunks)apps/web/prisma/migrations/20250414091625_rule_system_type/migration.sql(1 hunks)apps/web/prisma/schema.prisma(2 hunks)apps/web/utils/actions/rule.ts(13 hunks)apps/web/utils/actions/rule.validation.ts(2 hunks)apps/web/utils/ai/choose-rule/match-rules.ts(9 hunks)apps/web/utils/ai/choose-rule/types.ts(2 hunks)apps/web/utils/ai/reply/check-reply-tracking.ts(0 hunks)apps/web/utils/reply-tracker/enable.ts(4 hunks)apps/web/utils/rule/rule.ts(4 hunks)
💤 Files with no reviewable changes (1)
- apps/web/utils/ai/reply/check-reply-tracking.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/(app)/automation/onboarding/CategoriesSetup.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/utils/actions/rule.validation.ts
- apps/web/utils/ai/choose-rule/types.ts
- apps/web/prisma/schema.prisma
- apps/web/utils/actions/rule.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/web/utils/rule/rule.ts (1)
apps/web/utils/ai/rule/create-rule-schema.ts (1)
CreateOrUpdateRuleSchemaWithCategories(128-131)
apps/web/app/(app)/automation/onboarding/page.tsx (1)
apps/web/utils/actions/rule.validation.ts (1)
CategoryAction(155-155)
🔇 Additional comments (24)
apps/web/utils/ai/choose-rule/match-rules.ts (5)
1-1: The new imports properly support system type integrationThe added imports for Gmail API types, SystemType enum, calendar attachment detection, and sender reply history tracking align well with the PR objectives to extend rule matching functionality with system types.
Also applies to: 16-16, 30-31
41-51: Function parameter signature appropriately updatedThe addition of the Gmail API parameter is necessary to support the new functionality for checking sender reply history and is consistently applied across related functions.
57-74: Well-implemented calendar event detectionThe calendar event detection logic is well structured with appropriate logging. The implementation correctly identifies calendar events using the
hasIcsAttachmentfunction and returns a matching rule with the CALENDAR system type if found.
178-185: Function composition improves maintainabilityExtracting the TO_REPLY filtering logic into a separate function improves code maintainability and follows the single responsibility principle.
339-342:Details
✅ Verification successful
Verify the parameter order for checkSenderReplyHistory
Make sure the parameters for
checkSenderReplyHistoryare in the correct order. Double-check that the function expects the Gmail API instance first, then the sender email, and finally the threshold.
🏁 Script executed:
#!/bin/bash # Check the parameter order for checkSenderReplyHistory function rg -A 5 "function checkSenderReplyHistory" --glob="*.ts"Length of output: 658
Parameter Order Verified: No Changes Needed
After verifying, the parameter order for
checkSenderReplyHistoryis confirmed as follows:
gmail(Gmail API instance)senderEmail(sender email address)TO_REPLY_RECEIVED_THRESHOLD(reply received threshold)The code in
apps/web/utils/ai/choose-rule/match-rules.tsat lines 339-342 correctly aligns with the function definition inapps/web/utils/reply-tracker/check-sender-reply-history.ts.apps/web/prisma/migrations/20250414091625_rule_system_type/migration.sql (2)
4-4: Be aware of potential migration failure due to duplicates.This migration adds a unique constraint on
[userId,systemType]. If there are existing duplicate values in your database, this migration will fail. Make sure to clean up any duplicate data before running this in production.
8-8: LGTM: Well-structured enum creation.The
SystemTypeenum values cover a comprehensive set of system-defined rule categories, allowing for specialized rule handling based on their type.apps/web/app/(app)/automation/RuleForm.tsx (4)
34-34: LGTM: Proper import and type updates.The addition of
SystemTypeimport and update to useCoreConditionTypealigns with the system types feature implementation.Also applies to: 36-36
199-200: Type update for better type safety.Changing from
Exclude<ConditionType, "GROUP">toCoreConditionTypeimproves type safety and consistency with type definitions across the application.
269-275: Good feature: Conditional rendering of a badge for special preset rules.The added UI component clearly informs users when a rule has special preset logic that may impact conditions.
814-817: Improved description clarity for reply tracking.The updated description more clearly explains the purpose of the reply tracking action and its relationship with the
NEEDS_REPLY_LABEL_NAMElabel.apps/web/app/(app)/automation/onboarding/page.tsx (5)
5-10: LGTM: Updated imports to include SystemType.Proper import of the new SystemType enum from @prisma/client.
30-30: LGTM: Updated UserPreferences type to use systemType.Changed from selecting
nametosystemTypein the rules object, aligning with the database schema changes.
46-46: LGTM: Updated getUserPreferences to select systemType.This change ensures that the systemType field is retrieved from the database when getting user preferences.
62-66: LGTM: Updated preference functions to use SystemType enum constants.Using SystemType enum constants provides better type safety than string literals.
81-85: LGTM: Updated getRuleSetting to use SystemType.Modified the function parameter from
name: stringtosystemType: SystemTypeand updated the lookup logic to match rules based on systemType.apps/web/utils/rule/rule.ts (3)
4-9: LGTM: Updated imports to include SystemType.Proper import of the SystemType enum from @prisma/client.
34-34: LGTM: Added systemType parameter to safeCreateRule.This change allows passing the system type when creating a rule.
Also applies to: 46-46
108-108: LGTM: Added systemType to createRule function and data object.This properly adds the systemType field to the rule creation process.
Also applies to: 113-113, 121-121
apps/web/utils/reply-tracker/enable.ts (5)
3-3: Import statement updated to include SystemTypeThe import statement has been correctly updated to include
SystemTypefrom@prisma/client, which is necessary for the new system type filtering logic.
33-35: Query updated to filter by SystemTypeThe query has been updated to filter rules based on
systemTypewhich is more explicit and type-safe than the previous approach.
38-38: SystemType added to select statementAdding
systemTypeto the select statement ensures this field is available for processing rules.
53-53: Updated rule finding logicThe logic to find a rule has been updated to use the
systemTypefield, which aligns with the query changes above.
11-144: Overall changes look goodThe changes to use
SystemType.TO_REPLYfor rule identification provide a more structured and type-safe approach than the previous implementation. This aligns well with the PR objective of introducing presets functionality.Consider verifying that all error messages consistently use the current feature name throughout the codebase for better user experience.
apps/web/prisma/migrations/20250414091625_rule_system_type/migration.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/app/(app)/automation/examples.ts (1)
74-74: Fix the grammatical error in this line.There's redundant wording in "draft a reply a polite reply". Consider changing to either "draft a polite reply" or "draft a reply with a polite tone".
-* Label media inquiries as "Press" and draft a reply a polite reply.`, +* Label media inquiries as "Press" and draft a polite reply.`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(app)/automation/examples.ts(4 hunks)
🔇 Additional comments (4)
apps/web/app/(app)/automation/examples.ts (4)
20-24: LGTM! The removal of marketing and newsletter labels aligns with PR objectives.I notice the common prompts array has been streamlined, with marketing, newsletter, and receipt labeling prompts removed (as mentioned in the AI summary). This aligns with the PR's introduction of system types for categorization.
53-53: LGTM! The updated founder prompt is concise and relevant.The founder prompt now concludes with recruitment-related labeling, while the previously mentioned marketing, newsletter, and receipt labels have been removed (as per the AI summary).
91-91: LGTM! The investor persona prompt has been updated appropriately.The marketing-related labels have been removed as mentioned in the AI summary, and the prompt now concludes with a speaking opportunity categorization.
193-193: LGTM! The student prompt has been streamlined.As noted in the AI summary, the marketing and newsletter labeling instructions have been removed from the student persona prompt, which aligns with the PR's approach to handling these categorizations differently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/utils/ai/choose-rule/match-rules.ts (2)
35-35: Consider configuring the threshold.The
TO_REPLY_RECEIVED_THRESHOLD = 10might be better pulled from a configuration or environment variable to make it more flexible.-const TO_REPLY_RECEIVED_THRESHOLD = 10; +const TO_REPLY_RECEIVED_THRESHOLD = parseInt(process.env.TO_REPLY_RECEIVED_THRESHOLD ?? "10", 10);
320-367: Handle potential multiple TO_REPLY rules.Currently,
filterToReplyPresetonly filters out the first matching rule forSystemType.TO_REPLY. If there can be multipleTO_REPLYrules, consider filtering them all:-const toReplyRuleIndex = potentialMatches.findIndex( - (r) => r.systemType === SystemType.TO_REPLY, -); ... -if (toReplyRuleIndex === -1) { - return potentialMatches; -} ... -return potentialMatches.filter((_, index) => index !== toReplyRuleIndex); +const filteredMatches = potentialMatches.filter( + (r) => r.systemType !== SystemType.TO_REPLY +); ... +// If no prior reply and threshold reached, remove all TO_REPLY rules: +return filteredMatches;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/ai/choose-rule/match-rules.ts(10 hunks)
🔇 Additional comments (5)
apps/web/utils/ai/choose-rule/match-rules.ts (5)
1-1: New imports look correct.The newly introduced imports, including
gmail_v1,SystemType, and others from@prisma/client, seem consistent with the usage throughout the file.Also applies to: 12-17
56-74: Verify early return logic for calendar events.Returning immediately upon finding a
CALENDARrule prevents any other rules from being considered. Ensure that only one rule is expected to match in this flow.
178-184: Appropriate filtering approach for TO_REPLY.Integrating the
filterToReplyPresetcall before returning potential matches is a solid way to handle the new logic forTO_REPLYsystem types without cluttering this function further.
200-201: Log message for system preset detection is aligned.The
'Matched a system preset'return string correctly identifies thePRESETcondition type.
211-213:Details
✅ Verification successful
Method signatures updated seamlessly.
All additions of the
gmailparameter appear appropriate. Confirm that all upstream calling sites pass the correctgmailobject.Also applies to: 224-225, 236-237
🏁 Script executed:
#!/bin/bash # Verify that all invocations of `findMatchingRule` and `findMatchingRuleWithReasons` pass the `gmail` argument rg -A 2 "findMatchingRule\(" rg -A 2 "findMatchingRuleWithReasons\("Length of output: 4835
All invoking sites now pass the correct
gmailargumentThe verification confirms that every call to both
findMatchingRuleandfindMatchingRuleWithReasons—including those inapps/web/utils/ai/choose-rule/match-rules.test.tsandapps/web/utils/ai/choose-rule/run-rules.ts—consistently supplies thegmailparameter. The method signature updates are hence seamlessly integrated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/prisma/migrations/20250414103126_migrate_system_rule_types/migration.sql (1)
1-6: Ensure Atomicity and Enum Consistency.All six update statements correctly update the
"systemType"field with standardized uppercase enum values corresponding to the rule names. This adheres to the new enum definitions you have in your Prisma schema. Please verify that these values —'TO_REPLY','NEWSLETTER','MARKETING','CALENDAR','RECEIPT', and'NOTIFICATION'— exactly match the enums defined in your application.Additionally, if your migration system does not automatically wrap migration files in a transaction, consider manually wrapping these statements in a transaction (using
BEGIN;andCOMMIT;) to ensure atomicity during the migration process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/prisma/migrations/20250414103126_migrate_system_rule_types/migration.sql(1 hunks)
1 participant
Summary by CodeRabbit
.icsfile attachments.