WIP: Handle cold emails better across domains #989
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR enhances cold email filtering by introducing exclusion parameters ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant isColdEmail
participant EmailProvider
participant ExternalAPI
Client->>isColdEmail: isColdEmail(email, coldEmailRule)
Note over isColdEmail: Extract action label/folder<br/>from coldEmailRule
alt Has label (LABEL action)
isColdEmail->>EmailProvider: hasPreviousEmail(...,<br/>excludeLabel: label)
else Has folder (MOVE_FOLDER action)
isColdEmail->>EmailProvider: hasPreviousEmail(...,<br/>excludeFolder: folder)
else No rule
isColdEmail->>EmailProvider: hasPreviousEmail(...,<br/>excludeLabel: "Cold Email")
end
Note over EmailProvider: Build filtered query<br/>(exclude label/folder)
EmailProvider->>ExternalAPI: Search with exclusion filter
ExternalAPI-->>EmailProvider: Previous emails (filtered)
EmailProvider-->>isColdEmail: Has prior communication?
alt Prior non-cold email found
isColdEmail-->>Client: false (known sender)
else No prior emails
isColdEmail->>Client: Trigger AI classifier
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/utils/email/microsoft.ts (1)
1212-1220: Complex query construction - consider readability improvements.The filter query on line 1216 is quite long and combines multiple conditions:
- Outgoing emails (toRecipients) with date filter
- Incoming emails (from) with date filter
- Exclude filter appended to both parts
While functionally correct, the query construction could be more readable. Consider extracting the query building into a helper function or breaking it into smaller parts to improve maintainability and debuggability.
Example refactor:
const outgoingFilter = `toRecipients/any(a:a/emailAddress/address eq '${escapeODataString(options.from)}') and sentDateTime lt ${options.date.toISOString()}${excludeFilter}`; const incomingFilter = `from/emailAddress/address eq '${escapeODataString(options.from)}' and receivedDateTime lt ${options.date.toISOString()}${excludeFilter}`; const response: { value: Message[] } = await this.client .getClient() .api("/me/messages") .filter(`(${outgoingFilter}) or (${incomingFilter})`) .top(2) .select("id") .get();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/utils/cold-email/exclude-cold-emails.test.ts(1 hunks)apps/web/utils/cold-email/is-cold-email-label-exclusion.test.ts(1 hunks)apps/web/utils/cold-email/is-cold-email.ts(4 hunks)apps/web/utils/email/microsoft.ts(1 hunks)apps/web/utils/email/types.ts(1 hunks)apps/web/utils/gmail/message-label-exclusion.test.ts(1 hunks)apps/web/utils/gmail/message.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T04:19:57.099Z
Learnt from: edulelis
Repo: elie222/inbox-zero PR: 576
File: packages/resend/emails/digest.tsx:78-83
Timestamp: 2025-07-17T04:19:57.099Z
Learning: In packages/resend/emails/digest.tsx, the DigestEmailProps type uses `[key: string]: DigestItem[] | undefined | string | Date | undefined` instead of intersection types like `& Record<string, DigestItem[] | undefined>` due to implementation constraints. This was the initial implementation approach and cannot be changed to more restrictive typing.
Applied to files:
apps/web/utils/email/types.ts
🧬 Code graph analysis (5)
apps/web/utils/cold-email/is-cold-email.ts (2)
apps/web/utils/cold-email/cold-email-rule.ts (1)
ColdEmailRule(4-6)apps/web/utils/email.ts (1)
extractEmailAddress(19-52)
apps/web/utils/gmail/message-label-exclusion.test.ts (2)
apps/web/utils/email/types.ts (1)
EmailProvider(45-252)apps/web/utils/gmail/message.ts (1)
hasPreviousCommunicationsWithSenderOrDomain(244-268)
apps/web/utils/cold-email/is-cold-email-label-exclusion.test.ts (4)
apps/web/__tests__/helpers.ts (1)
getEmailAccount(26-46)apps/web/utils/cold-email/cold-email-rule.ts (1)
ColdEmailRule(4-6)apps/web/utils/types.ts (1)
EmailForLLM(120-134)apps/web/utils/cold-email/is-cold-email.ts (1)
isColdEmail(20-99)
apps/web/utils/email/microsoft.ts (2)
apps/web/utils/outlook/folders.ts (1)
getOrCreateOutlookFolderIdByName(127-161)apps/web/utils/outlook/odata-escape.ts (1)
escapeODataString(13-20)
apps/web/utils/cold-email/exclude-cold-emails.test.ts (3)
apps/web/utils/email/types.ts (1)
EmailProvider(45-252)apps/web/__tests__/helpers.ts (1)
getEmailAccount(26-46)apps/web/utils/cold-email/is-cold-email.ts (1)
isColdEmail(20-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Jit Security
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/web/utils/cold-email/exclude-cold-emails.test.ts (1)
1-346: Excellent test coverage for label/folder exclusion logic.The test suite is well-structured and covers all critical scenarios:
- Gmail label exclusion with default and custom labels
- Microsoft Outlook distinction between LABEL (category) and MOVE_FOLDER actions
- Fallback to default "Cold Email" label when no rule is configured
- Previous email detection with and without exclusions
- Short-circuit behavior when date/id is missing
The tests effectively validate that the exclusion parameters are correctly passed through to the provider layer.
apps/web/utils/email/types.ts (1)
220-221: Well-documented interface extension.The addition of
excludeLabelandexcludeFolderparameters is clearly documented with inline comments explaining provider-specific usage. The nullable types provide appropriate flexibility for different scenarios.apps/web/utils/cold-email/is-cold-email-label-exclusion.test.ts (1)
1-323: Comprehensive test coverage for label exclusion.The test suite thoroughly validates:
- Default "Cold Email" label usage when rule is null or has no actions
- Custom label name propagation
- Correct parameter passing to the provider
- Early return for known cold emailers (avoiding unnecessary provider calls)
- Integration with previous email detection logic
The tests effectively ensure the label exclusion feature works correctly across different configurations.
apps/web/utils/gmail/message-label-exclusion.test.ts (1)
1-266: Thorough Gmail query construction tests.Excellent test coverage for Gmail-specific label exclusion:
- Validates correct query syntax (
-label:"Label Name")- Tests domain-based searches for company emails vs full email for public domains
- Verifies both incoming (from:) and outgoing (to:) query construction
- Ensures current message is properly excluded from results
The tests effectively validate the Gmail implementation details.
apps/web/utils/cold-email/is-cold-email.ts (1)
59-74: Clean implementation of label/folder exclusion logic.The logic correctly:
- Extracts the action from the cold email rule
- Determines whether to exclude by folder (MOVE_FOLDER) or label (LABEL)
- Defaults to "Cold Email" when no action is configured
- Passes appropriate parameters to the provider
The fallback to "Cold Email" (line 61) ensures consistent behavior even when rules are not fully configured, which aligns with the test expectations.
apps/web/utils/gmail/message.ts (2)
173-204: Clean implementation of Gmail label exclusion.The implementation correctly:
- Adds the
excludeLabelparameter tofindPreviousEmailsWithSender- Constructs the exclusion query only when
excludeLabelis provided- Appends the exclusion to both incoming (from:) and outgoing (to:) searches
- Maintains consistent query construction
The logic properly threads the exclusion through both email directions, ensuring comprehensive filtering.
244-268: Proper propagation of exclusion parameters.The function correctly:
- Accepts both
excludeLabelandexcludeFolderparameters (though Gmail doesn't useexcludeFolder)- Determines whether to search by domain or full email based on public domain check
- Passes
excludeLabelthrough tohasPreviousCommunicationWithSenderThe domain-based search logic for company emails vs. public email providers is well-implemented and will improve cold email detection across different sender types.
| let excludeFilter = ""; | ||
|
|
||
| if (options.excludeFolder) { | ||
| // Exclude by folder (for MOVE_FOLDER action) | ||
| try { | ||
| const folderId = await getOrCreateOutlookFolderIdByName( | ||
| this.client, | ||
| options.excludeFolder, | ||
| ); | ||
| if (folderId) { | ||
| excludeFilter = ` and parentFolderId ne '${folderId}'`; | ||
| } | ||
| } catch (error) { | ||
| // If folder doesn't exist or we can't get it, proceed without filtering | ||
| this.logger.info("Could not get folder ID for exclusion", { | ||
| folder: options.excludeFolder, | ||
| error, | ||
| }); | ||
| } | ||
| } else if (options.excludeLabel) { | ||
| // Exclude by category (for LABEL action) | ||
| const categoryName = escapeODataString(options.excludeLabel); | ||
| excludeFilter = ` and not (categories/any(c: c eq '${categoryName}'))`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the OData category exclusion syntax.
The category exclusion query uses:
excludeFilter = ` and not (categories/any(c: c eq '${categoryName}'))`;This looks correct for OData, but please verify that the not (categories/any(...)) syntax is the standard approach for excluding messages with a specific category in Microsoft Graph API.
Also, the folder resolution silently continues without exclusion if it fails (lines 1200-1205). Consider whether this should be logged at a higher level (warn instead of info) or handled differently to ensure developers are aware when folder exclusion is skipped.
🌐 Web query:
Does Microsoft Graph API support "not (categories/any(c: c eq 'value'))" syntax for excluding emails with specific categories?
💡 Result:
Short answer: Yes — Microsoft Graph's $filter supports using not with the lambda any() operator (e.g. not categories/any(c:c eq 'Value')), but behavior can vary by resource and there are known limitations/quirks.
Evidence:
- The Graph $filter docs show lambda operators and the not(...) pattern for collections (examples such as not(groupTypes/any(...))). [1]
- A Microsoft Q&A post demonstrates using not categories/any() to return messages with no categories. [2]
- However, community threads report inconsistent behavior for more complex category filters (for example AND between multiple any() expressions sometimes returns empty results), so expect edge cases and test your exact query. [3][4]
If you need, tell me the exact query and mailbox/folder you’re using and I’ll help test/adjust it.
Sources:
[1] Microsoft Graph: Use the $filter query parameter.
[2] Microsoft Q&A: Filter Emails That Do Not Have An Assigned Category Using Graph API.
[3] Microsoft Q&A / forum threads on filtering messages by categories.
[4] StackOverflow: querying mails without categories not supported.
OData category exclusion syntax is supported; consider edge case testing and document the graceful fallback behavior.
The not (categories/any(c: c eq '${categoryName}')) syntax is valid and documented in Microsoft Graph's $filter API. However, community reports note inconsistent behavior for more complex category filters, so test this query in your specific scenarios (especially if multiple categories or complex AND conditions are involved).
Regarding folder resolution failure (lines 1200-1205): The current approach logs at info level and gracefully continues without filtering. This seems intentional, but consider clarifying in a code comment why silent fallback is preferred here—specifically whether skipping exclusion is always safe or if certain failure modes should be treated more seriously. If folder IDs become stale or resolution failures indicate real data consistency issues, an info level might mask problems. Document the contract: "if folder doesn't exist, exclusion is skipped; caller must handle this."
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 1187 to 1210, the OData
category exclusion syntax is correct but you must add explicit documentation +
handling for edge cases and clarify the folder-resolution fallback: add a
concise code comment explaining the contract ("if folder doesn't exist or cannot
be resolved, exclusion is skipped and caller must handle potential stale folder
IDs"), and annotate which failure modes should remain info vs escalate (e.g.,
transient network errors vs permission/consistency errors). Also add or update a
unit/integration test note to exercise category filters with multiple
categories/complex conditions and a TODO to verify Graph behavior in those
scenarios, and consider changing the logger level to warn or include error.code
when folder resolution fails for non-expected errors.
| const excludeLabelQuery = options.excludeLabel | ||
| ? ` -label:"${options.excludeLabel}"` | ||
| : ""; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify label name escaping in Gmail queries.
The label exclusion query is constructed using a template literal without escaping:
const excludeLabelQuery = options.excludeLabel
? ` -label:"${options.excludeLabel}"`
: "";If the label name contains special characters (like quotes, backslashes, or other Gmail search operators), this could:
- Break the query syntax
- Potentially allow query injection if label names come from untrusted sources
Please verify whether Gmail label names need escaping and, if so, implement appropriate escaping logic.
🏁 Script executed:
#!/bin/bash
# Search for existing escaping utilities in the Gmail codebase
echo "=== Searching for Gmail query escaping functions ==="
rg -n "escape.*gmail|gmail.*escape" --type ts -g '!node_modules' -g '!dist' -C 3
echo -e "\n=== Searching for label name sanitization ==="
rg -n "label.*escape|escape.*label|sanitize.*label" --type ts -g '!node_modules' -g '!dist' -C 3
echo -e "\n=== Check existing query construction patterns ==="
ast-grep --pattern 'query: `from:$_' -l tsLength of output: 151
I need to investigate further to understand the actual risk. Let me search for how excludeLabel is used and sourced in the codebase:
Let me search for where the excludeLabel parameter comes from and how it's used:
Now let me search for where the excludeLabel parameter comes from and how it's validated:
🏁 Script executed:
#!/bin/bash
# Find where excludeLabel parameter is used and defined
echo "=== Searching for excludeLabel usage ==="
rg -n "excludeLabel" --type ts -C 5 -g '!node_modules' -g '!dist'
echo -e "\n=== Search for the function signature and options interface ==="
rg -n "excludeLabel\|interface.*options" --type ts -B 5 -A 10 -g '!node_modules' -g '!dist' | head -100Length of output: 30993
Add escaping for label names in Gmail search queries.
While some tools restrict label names from containing special characters like forward slashes and backslashes, Gmail's own API doesn't enforce all these restrictions uniformly. The current code constructs label exclusion queries without escaping:
const excludeLabelQuery = options.excludeLabel
? ` -label:"${options.excludeLabel}"`
: "";If a label name contains quotation marks (e.g., My "Special" Label), the query breaks:
-label:"My "Special" Label"
becomes malformed at the first internal quote.
Additionally, apostrophes and special characters can be tricky in Gmail search and may require escaping with a backslash.
Implement escaping for label names to handle quotes and other special characters properly. The Outlook implementation in microsoft.ts shows that similar escaping logic exists elsewhere in the codebase (line 1208 uses escapeODataString), indicating this concern has been recognized for other providers.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
When someone emails us from the same domain, but under a different email, we skip the cold email check.
A better way to handle this is to only search for previous emails from the domain in question, that aren't labelled cold email. This PR does that.
Needs to be checked more before it can be merged in. And code may need clean up too.
Summary by CodeRabbit
Tests
Refactor