Fix outlook cold email #1002
Fix outlook cold email #1002
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces a single inline OData filter in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant MicrosoftUtil as microsoft.ts
participant Graph as Graph API
Caller->>MicrosoftUtil: hasPreviousCommunicationsWithSenderOrDomain(msgId, from, domain, date)
Note right of MicrosoftUtil: escape `from`, build sentFilter and receivedFilter
par parallel queries
MicrosoftUtil->>Graph: query(sentFilter) (async)
MicrosoftUtil->>Graph: query(receivedFilter) (async)
end
Graph-->>MicrosoftUtil: sentResults / sentError
Graph-->>MicrosoftUtil: receivedResults / receivedError
Note right of MicrosoftUtil: aggregate results, log per-path errors, filter out current msgId
MicrosoftUtil-->>Caller: boolean (any previous message found)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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)
1179-1240: Optional: Method name implies domain checking but only checks exact sender.The method is named
hasPreviousCommunicationsWithSenderOrDomainbut the implementation only checks for communications with the exact sender email address, not the domain.If domain-level checking is intended, consider extracting the domain from
options.fromand adding an additional query to check for communications with any address at that domain. Otherwise, consider renaming the method tohasPreviousCommunicationsWithSenderto match its actual behavior.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/scripts/check-enum-imports.js(0 hunks)apps/web/utils/email/microsoft.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/scripts/check-enum-imports.js
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/utils/email/microsoft.ts (1)
apps/web/utils/outlook/odata-escape.ts (1)
escapeODataString(13-20)
⏰ 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Review for correctness
| // Split into two parallel queries to avoid OData "invalid nodes" error | ||
| // when combining any() lambda with other filters. | ||
| const receivedFilter = `from/emailAddress/address eq '${escapedFrom}' and receivedDateTime lt ${dateString}`; | ||
|
|
||
| // Use $search for sent messages as $filter on toRecipients is unreliable | ||
| const sentSearch = `"to:${options.from}"`; | ||
|
|
There was a problem hiding this comment.
Critical: Add date filtering to sent messages query.
The sentSearch query doesn't filter by date, meaning it will find sent messages to this sender at ANY point in history. This breaks the cold email detection logic—if you sent an email to someone last year but they're contacting you for the first time today, this will incorrectly return true (prior communications found).
Apply this diff to add date filtering to the sent messages query:
- // Use $search for sent messages as $filter on toRecipients is unreliable
- const sentSearch = `"to:${options.from}"`;
+ // Use $filter for sent messages with date constraint
+ // We need to filter sent messages to this recipient before the current message date
+ const sentFilter = `toRecipients/any(r: r/emailAddress/address eq '${escapedFrom}') and receivedDateTime lt ${dateString}`;And update the query to use .filter() instead of .search():
const [sentResponse, receivedResponse] = await Promise.all([
this.client
.getClient()
.api("/me/messages")
- .search(sentSearch)
+ .filter(sentFilter)
.top(2)
.select("id")
.get()
.catch((error) => {
this.logger.error("Error checking sent messages", {
error,
- search: sentSearch,
+ filter: sentFilter,
});
return { value: [] };
}),Note: The PR description mentions this change was made to "fix outlook cold email" but the current implementation still has this date filtering gap for sent messages. If there's a specific reason $search must be used instead of $filter (as the original comment suggests), please add date filtering to the search query or document why it's not needed.
Committable suggestion skipped: line range outside the PR's diff.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="apps/web/utils/email/microsoft.ts">
<violation number="1" location="apps/web/utils/email/microsoft.ts:1199">
The sent-messages lookup runs without any receivedDateTime constraint, so any message you have ever sent to this recipient will cause the cold-email check to return true. Constrain the sent query to messages earlier than the current message date so historical conversations outside the cutoff do not trigger the prior-communication check.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/utils/email/microsoft.ts(1 hunks)
⏰ 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: cubic · AI code reviewer
- GitHub Check: test
- GitHub Check: Review for correctness
- GitHub Check: Analyze (javascript-typescript)
| const escapedSearchFrom = options.from | ||
| .replace(/\\/g, "\\\\") | ||
| .replace(/"/g, '\\"'); | ||
| const sentSearch = `"to:${escapedSearchFrom}"`; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify KQL escaping completeness and "to:" syntax reliability.
The manual escaping for the KQL search query only handles backslashes and double quotes. KQL supports additional special characters and operators (parentheses, AND, OR, NOT, wildcards, etc.) that could appear in email addresses or display names. While wrapping the query in double quotes provides some protection, edge cases like user(name)@example.com or emails containing KQL keywords might still cause unexpected behavior.
Additionally, verify that the to: property filter syntax is reliable for the Microsoft Graph $search API, as the previous comment mentions that $filter on toRecipients is unreliable.
Consider running this verification to check if the current escaping handles various email formats correctly:
Alternatively, search the web for Microsoft Graph KQL search syntax requirements:
🏁 Script executed:
#!/bin/bash
# Search for any usage patterns or tests related to KQL search escaping
# to understand what edge cases are already handled
rg -n -C3 "\.search\(" --type=ts
# Also check for any existing documentation or comments about KQL syntax
rg -n -C3 "KQL|Keyword Query" --type=tsLength of output: 3108
🌐 Web query:
Microsoft Graph $search KQL syntax escaping requirements for email addresses
💡 Result:
Short answer — follow the Graph docs: put each KQL clause inside double quotes, escape any double-quote or backslash characters with a backslash, and URL‑encode any other special characters in the clause (e.g. @ → %40, + → %2B). Also include ConsistencyLevel: eventual when needed. Example requests:
-
Single clause (URL-encoded @):
GET https://graph.microsoft.com/v1.0/me/messages?$search="from:someone%40contoso.com". (learn.microsoft.com) -
If your clause contains a literal double-quote or backslash, escape them with a backslash inside the quoted clause (per docs). (learn.microsoft.com)
Notes:
- If you put raw special characters (un-encoded) in the URL you may get KQL syntax errors (see reported error for from:[email protected]). URL‑encode characters like @, +, & etc. to avoid the error. (stackoverflow.com)
If you want, tell me an example email or full request you’re trying and I’ll give the exact encoded $search string.
Citations:
- 1: https://learn.microsoft.com/en-us/graph/search-query-parameter?tabs=csharp&utm_source=openai
- 2: https://learn.microsoft.com/en-us/graph/search-query-parameter?tabs=csharp&utm_source=openai
- 3: https://stackoverflow.com/questions/79766312/microsoft-graph-api-search-fails-with-syntax-error-character-is-not-valid?utm_source=openai
This would search for existing test patterns related to email and special characters that might shed light on whether the current approach is working or if there are known issues.
However, based on the evidence gathered so far, I can now make a determination:
**Evidence Summary:**
1. Per RFC 3986, query parameter values must be percent-encoded, and many HTTP clients handle this encoding automatically
2. There is a known issue with encoding ampersand (&) symbols in $search expressions on the v1.0 endpoint
3. The code only escapes backslashes and double quotes—it does NOT perform URL encoding (like @ → %40)
4. According to the first web search, Microsoft Graph documentation specifies that special characters like @ must be URL-encoded as %40 in KQL search clauses
The review comment is substantively correct: the escaping is incomplete. However, I need to verify if this is actually causing issues in practice or if the SDK handles it automatically.
Let me search for how email addresses are typically used in practice:
This will show if there are any existing concerns or test cases around this functionality.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for any tests related to email search with special characters
# or any existing issues/TODOs related to email escaping
rg -n "email|@" --type=ts apps/web/__tests__ | grep -i "search\|special\|escape"
# Also look for any OData/KQL filtering patterns in the codebase
rg -n "to:" --type=ts
# Check if there are any tests for the microsoft.ts file specifically
fd -t f "microsoft.*test" apps/web/__tests__
Length of output: 34681
🏁 Script executed:
#!/bin/bash
# Look for actual test cases or usage of email addresses with the search functionality
rg -n -B5 -A5 "sentSearch|searchFrom" apps/web --type=ts
# Check if there are any GitHub issues or TODOs mentioning email encoding
rg -n "TODO|FIXME|BUG|XXX" apps/web/utils/email/microsoft.tsLength of output: 1784
🌐 Web query:
@microsoft/microsoft-graph-client .search() method URL encoding parameter handling
💡 Result:
Short answer: .search() does not perform URL percent-encoding for you — it just sets the $search value you pass onto the request. You must URL‑encode (and in some cases double‑encode) any reserved characters (e.g. & , spaces, parentheses, quotes) before calling .search(). See Microsoft Graph query-parameter encoding guidance and the client docs showing .search() simply adds the string. [1][2][3]
Quick guidance:
- Use encodeURIComponent(searchString) when building the value you pass to .search().
- For $search expressions with characters that Graph requires double-encoding (per Microsoft docs), apply the additional encoding as described in the docs.
- If you need full control you can also use .query("$search=" + encodedString) to supply the exact query string. [1][2][3]
Sources:
[1] Microsoft Graph query-parameter encoding and $search notes.
[2] @microsoft/microsoft-graph-client npm docs (query / $search behavior).
[3] jsDocs for @microsoft/microsoft-graph-client showing .search() behavior.
URL-encode email addresses in KQL $search queries—current code will fail for all searches.
Per Microsoft Graph client documentation, .search() does not perform URL percent-encoding automatically; you must URL-encode special characters like @ before passing the value to .search(). Email addresses always contain @, which must be encoded as %40.
The current implementation (lines 1194–1197) only escapes backslashes and double quotes but does NOT URL-encode the email address. This will cause KQL syntax errors at runtime for every search, resulting in the error handler silently returning an empty array and causing hasPreviousCommunicationsWithSenderOrDomain to incorrectly return false.
Fix: Apply URL encoding (e.g., encodeURIComponent()) to the email address after escaping quotes and backslashes, before constructing the KQL search query.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 1194 to 1197, the code only
escapes backslashes and quotes but does not URL-encode the email address for the
KQL .search() call, so "@" and other special characters break the query; update
the flow to first escape backslashes and double quotes, then apply URL encoding
(e.g., encodeURIComponent) to the resulting string before interpolating it into
the KQL search expression so the email is percent-encoded (@" -> %40) and the
.search() call will succeed.
| const [sentResponse, receivedResponse] = await Promise.all([ | ||
| this.client | ||
| .getClient() | ||
| .api("/me/messages") | ||
| .search(sentSearch) | ||
| .top(5) // Increase top to account for potential future messages we filter out | ||
| .select("id,sentDateTime") | ||
| .get() | ||
| .catch((error) => { | ||
| this.logger.error("Error checking sent messages", { | ||
| error, | ||
| search: sentSearch, | ||
| }); | ||
| return { value: [] }; | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: .top(5) limit risks false negatives when combined with client-side date filtering.
The sent messages query fetches only the top 5 results without date filtering (since $search doesn't support it), then filters by date client-side (lines 1231-1237). If someone has sent more than 5 messages to this recipient and the 5 most recent are all after options.date, the query will fetch only those newer messages. The client-side filter will then exclude all 5, resulting in zero sent messages found—even if older sent messages (before the date) exist.
This breaks the cold email detection logic: the method would incorrectly return false (no prior communication) when prior communication actually occurred.
Apply this diff to significantly increase the limit and reduce false negative risk:
- .top(5) // Increase top to account for potential future messages we filter out
+ .top(50) // Fetch more results to account for future messages filtered out client-sideNote: The ideal solution would be to use $filter with date constraints instead of $search, but the comment on line 1192 indicates $filter on toRecipients is unreliable. If that's no longer true, consider switching to $filter with a receivedDateTime constraint.
Verify whether $filter on toRecipients is still unreliable:
🌐 Web query:
Microsoft Graph API filter toRecipients reliability 2025
💡 Result:
Short answer: you can't reliably use $filter on toRecipients. Microsoft Graph does not support filtering on toRecipients/ccRecipients/bccRecipients (they're collection-of-complex-types), so use $search (e.g. ?$search="to:[[email protected]]" or ?$search="participants:[[email protected]]") or retrieve and filter client-side. [1][2][3]
Notes/limits:
- $search is the recommended workaround but has its own limits (e.g. result caps per folder/search in some implementations). [4]
Sources:
- Microsoft Tech Community discussion (toRecipients not filterable). [1]
- Microsoft Q&A answer (confirming toRecipients cannot be filtered; use $search). [2]
- StackOverflow thread and examples showing same behavior and $search workaround. [3]
- Docs/notes on search limits (result caps / practical caveats). [4]
Critical: .top(5) limit risks false negatives when combined with client-side date filtering.
The sent messages query fetches only the top 5 results using $search, then filters by date client-side (lines 1231-1237). If someone has sent more than 5 messages to this recipient and the 5 most recent are all after options.date, the query will fetch only those newer messages. The client-side filter will then exclude all 5, resulting in zero sent messages found—even if older sent messages (before the date) exist.
This breaks the cold email detection logic: the method would incorrectly return false (no prior communication) when prior communication actually occurred.
Apply this diff to significantly increase the limit and reduce false negative risk:
- .top(5) // Increase top to account for potential future messages we filter out
+ .top(50) // Fetch more results to account for future messages filtered out client-sideNote: Microsoft Graph does not support $filter on toRecipients (it's a collection-of-complex-type), so the current $search approach is the recommended workaround. Consider increasing .top() further if the organization typically exchanges many messages with the same recipients.
📝 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.
| const [sentResponse, receivedResponse] = await Promise.all([ | |
| this.client | |
| .getClient() | |
| .api("/me/messages") | |
| .search(sentSearch) | |
| .top(5) // Increase top to account for potential future messages we filter out | |
| .select("id,sentDateTime") | |
| .get() | |
| .catch((error) => { | |
| this.logger.error("Error checking sent messages", { | |
| error, | |
| search: sentSearch, | |
| }); | |
| return { value: [] }; | |
| }), | |
| const [sentResponse, receivedResponse] = await Promise.all([ | |
| this.client | |
| .getClient() | |
| .api("/me/messages") | |
| .search(sentSearch) | |
| .top(50) // Fetch more results to account for future messages filtered out client-side | |
| .select("id,sentDateTime") | |
| .get() | |
| .catch((error) => { | |
| this.logger.error("Error checking sent messages", { | |
| error, | |
| search: sentSearch, | |
| }); | |
| return { value: [] }; | |
| }), |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 1199 to 1213, the query limits
sent messages to .top(5) which combined with client-side date filtering can
produce false negatives when a recipient has more than 5 recent messages;
increase the .top(...) value to a much larger number (e.g., 50 or 100) or make
it a configurable constant/ENV value, keep the existing client-side date filter,
and add a short comment explaining why the higher limit is required (to avoid
missing older matches due to $search limitations on toRecipients).
| .catch((error) => { | ||
| this.logger.error("Error checking sent messages", { | ||
| error, | ||
| search: sentSearch, | ||
| }); | ||
| return { value: [] }; | ||
| }), | ||
|
|
||
| this.client | ||
| .getClient() | ||
| .api("/me/messages") | ||
| .filter(receivedFilter) | ||
| .top(2) | ||
| .select("id") | ||
| .get() | ||
| .catch((error) => { | ||
| this.logger.error("Error checking received messages", { | ||
| error, | ||
| filter: receivedFilter, | ||
| }); | ||
| return { value: [] }; | ||
| }), |
There was a problem hiding this comment.
Error handling silently masks failures, risking false negatives.
Both queries return { value: [] } on error, treating API failures, network issues, or malformed queries the same as "no messages found." This can cause the method to incorrectly return false when the real issue is a transient error or a query syntax problem.
Consider re-throwing errors or logging warnings more prominently, so that failures are visible and can be investigated rather than silently treated as "no prior communication."
Apply this diff to improve error visibility:
.catch((error) => {
this.logger.error("Error checking sent messages", {
error,
search: sentSearch,
});
+ // Re-throw on non-transient errors to avoid silent false negatives
+ // For now, return empty to maintain existing behavior but consider revisiting
return { value: [] };
}),
this.client
.getClient()
.api("/me/messages")
.filter(receivedFilter)
.top(2)
.select("id")
.get()
.catch((error) => {
this.logger.error("Error checking received messages", {
error,
filter: receivedFilter,
});
+ // Re-throw on non-transient errors to avoid silent false negatives
+ // For now, return empty to maintain existing behavior but consider revisiting
return { value: [] };
}),Alternatively, add monitoring/alerting on these error logs to ensure failures are investigated.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 1207 to 1228, the catch blocks
currently log the error and return { value: [] }, which masks real
API/network/query failures as "no messages"; change these handlers to log the
error and then re-throw (or return a rejected promise) so callers can
distinguish failures from empty results (alternatively propagate a descriptive
Error with context such as "Error checking sent/received messages" and the
filter/search), ensuring monitoring/alerts can detect and surface these failures
rather than silently treating them as empty responses.
Fix Outlook cold email detection by splitting queries in
OutlookProvider.hasPreviousCommunicationsWithSenderOrDomainin microsoft.ts and updating version to v2.20.19Reworks prior communication checks to run parallel sent
$searchwith KQLto:and received$filterqueries, applies client-side date filtering for sent results, merges sets excluding currentmessageId, and adds per-query error handling. Updates version to v2.20.19.📍Where to Start
Start with
OutlookProvider.hasPreviousCommunicationsWithSenderOrDomainin microsoft.ts.Macroscope summarized 51f03ca.
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.