Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis change extracts the Gmail and Outlook email provider implementations from a monolithic provider file into dedicated modules, introduces a shared Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProviderFactory as createEmailProvider
participant GmailProvider
participant OutlookProvider
Caller->>ProviderFactory: createEmailProvider(provider, client)
alt provider is "google"
ProviderFactory->>GmailProvider: new GmailProvider(client)
ProviderFactory-->>Caller: GmailProvider instance
else provider is "microsoft-entra-id"
ProviderFactory->>OutlookProvider: new OutlookProvider(client)
ProviderFactory-->>Caller: OutlookProvider instance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (12)
apps/web/utils/reply-tracker/check-sender-reply-history.ts (1)
21-25: Inconsistent defaults: comment says “reply might be needed” but code sets hasReplied=trueBoth the invalid-sender branch and error fallbacks set hasReplied to true, which contradicts the intent (“assume a reply might be needed”). Flip these to false.
Apply this diff:
@@ 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 }; + return { hasReplied: false, receivedCount: 0 }; } @@ - ]).catch((error) => { + ]).catch((error) => { logger.error("Timeout or error in parallel operations", { error, cleanSenderEmail, }); - return [true, 0] as const; // Safe defaults + return [false, 0] as const; // Safe defaults: assume reply might be needed }); @@ } catch (error) { // Catch potential errors from Promise.all or other unexpected issues logger.error("Overall error checking sender reply history", { error, senderEmail, cleanSenderEmail, }); // Default to assuming a reply might be needed on error - return { hasReplied: true, receivedCount: 0 }; + return { hasReplied: false, receivedCount: 0 }; }Also applies to: 32-38, 55-56
apps/web/app/api/user/stats/newsletters/route.ts (1)
207-213: Use safer Prisma raw query APIs or tighten current usageCurrent implementation uses $queryRawUnsafe with manual placeholders. While inputs are constrained (orderBy via enum and limit via number coercion), prefer the safer API to avoid footguns.
Option A (minimal): Remove unused spread of query.values and keep validated params.
- const results = await prisma.$queryRawUnsafe<NewsletterCountRawResult[]>( - query.sql, - ...queryParams, - ...query.values, - ); + const results = await prisma.$queryRawUnsafe<NewsletterCountRawResult[]>( + query.sql, + ...queryParams, + );Option B (preferred): Build the SQL with Prisma.sql interpolations (no manual $1-style placeholders) and call prisma.$queryRaw(query). If you want, I can provide a full rewrite diff.
apps/web/utils/assistant/process-assistant-email.ts (1)
236-239: Guard against null from extractEmailAddress to avoid runtime errorextractEmailAddress can return null/undefined; calling .toLowerCase() would throw.
Apply this diff:
function verifyUserSentEmail({ message, userEmail, }: { message: ParsedMessage; userEmail: string; }) { - return ( - extractEmailAddress(message.headers.from).toLowerCase() === - userEmail.toLowerCase() - ); + const from = extractEmailAddress(message.headers.from); + if (!from) return false; + return from.toLowerCase() === userEmail.toLowerCase(); }apps/web/utils/gmail/message.ts (2)
213-227: Bug: PUBLIC_DOMAINS contains entries with a leading '@'extractDomainFromEmail returns domains without '@', so "@me.com" and "@hey.com" will never match, breaking domain-based filtering.
Apply this diff:
"icloud.com", - "@me.com", + "me.com", "protonmail.com", "zoho.com", "yandex.com", "fastmail.com", "gmx.com", - "@hey.com", + "hey.com",
168-195: Provider-agnostic logic should not live under gmail/utilsfindPreviousEmailsWithSender and hasPrevious* functions depend on the generic EmailProvider and are not Gmail-specific. Keeping them in apps/web/utils/gmail/ violates the “provider-specific details stay isolated” rule.
Consider moving these functions to a provider-agnostic location, e.g. apps/web/utils/email/communication.ts (or similar), and re-export if needed.
Pros:
- Enforces separation of concerns
- Avoids Gmail coupling for Outlook (and future) providers
Cons:
- Small refactor and import path updates
Also applies to: 197-211, 229-246
apps/web/app/api/threads/[id]/route.ts (2)
41-55: Remove try/catch in GET route wrapped by middlewareGuidelines: “Do not use try/catch in GET API routes when using authentication middleware; rely on centralized error handling.”
Apply:
- try { - const thread = await getThread(id, includeDrafts, emailProvider); - return NextResponse.json(thread); - } catch (error) { - logger.error("Error fetching thread", { - error, - emailAccountId, - threadId: id, - }); - return NextResponse.json( - { error: "Failed to fetch thread" }, - { status: 500 }, - ); - } + const thread = await getThread(id, includeDrafts, emailProvider); + return NextResponse.json(thread);
20-23: Introduce provider-agnostic draft detection
Hardcoded checks against"DRAFT"assume Gmail semantics and won’t correctly filter Outlook drafts. Let’s extend theEmailProviderinterface with anisDraftMessage(msg: ParsedMessage): booleanmethod and delegate all draft checks to each provider.Key changes:
- apps/web/app/api/threads/[id]/route.ts
- const filteredMessages = includeDrafts
- ? thread.messages
- : thread.messages.filter((msg) => !msg.labelIds?.includes("DRAFT"));
- const filteredMessages = includeDrafts
- ? thread.messages
- : thread.messages.filter((msg) => !emailProvider.isDraftMessage(msg));
- Update EmailProvider interface (`apps/web/utils/email/types.ts`) ```ts export interface EmailProvider { // … /** Return true if this message is a draft in the current provider */ isDraftMessage(message: ParsedMessage): boolean; }
- Implement
isDraftMessagein each provider:
• Gmail (apps/web/utils/gmail/message.ts):• Outlook (isDraftMessage: (msg) => msg.labelIds?.includes(GmailLabel.DRAFT) ?? falseapps/web/utils/outlook/message.ts):isDraftMessage: (msg) => msg.isDraft === true- Replace all other literal draft filters (
.labelIds?.includes("DRAFT")ormessage.isDraft) in:
• apps/web/utils/gmail/thread.ts
• apps/web/components/email-list/EmailThread.tsx
• apps/web/utils/actions/stats.ts
• apps/web/app/api/messages/route.tsThis ensures consistent draft detection across providers.
apps/web/utils/ai/actions.ts (1)
263-266: Avoid any in ActionFunction typeTighten args typing; only id is used.
-const digest: ActionFunction<any> = async ({ email, emailAccountId, args }) => { - const actionId = args.id; +const digest: ActionFunction<{ id?: string | null }> = async ({ + email, + emailAccountId, + args, +}) => { + const actionId = args.id ?? undefined; await enqueueDigestItem({ email, emailAccountId, actionId }); };apps/web/utils/cold-email/is-cold-email.ts (2)
162-171: Bug: findUnique used with non-unique fields (status) — switch to findFirst or adjust logicPrisma findUnique must use only unique fields in where. Including status makes this invalid and will throw at runtime/compile time. Use findFirst with scalar fields, or use findUnique on the composite and check status in code.
Use findFirst with full filter:
- const coldEmail = await prisma.coldEmail.findUnique({ - where: { - emailAccountId_fromEmail: { - emailAccountId, - fromEmail: from, - }, - status: ColdEmailStatus.AI_LABELED_COLD, - }, - select: { id: true }, - }); + const coldEmail = await prisma.coldEmail.findFirst({ + where: { + emailAccountId, + fromEmail: from, + status: ColdEmailStatus.AI_LABELED_COLD, + }, + select: { id: true }, + });
59-67: Harden provider call with try/catchIf the provider throws, we should not fail the entire flow. Log and fall back to AI check.
- const hasPreviousEmail = - email.date && email.id - ? await provider.hasPreviousCommunicationsWithSenderOrDomain({ - from: email.from, - date: email.date, - messageId: email.id, - }) - : false; + let hasPreviousEmail = false; + if (email.date && email.id) { + try { + hasPreviousEmail = + await provider.hasPreviousCommunicationsWithSenderOrDomain({ + from: email.from, + date: email.date, + messageId: email.id, + }); + } catch (error) { + logger.warn("Previous communications check failed", { + ...loggerOptions, + error, + }); + } + }apps/web/utils/reply-tracker/outbound.ts (1)
141-146: Guard provider.getThreadMessages with try/catchA provider failure here should not crash the handler; log and return early.
- const threadMessages = await provider.getThreadMessages(message.threadId); - if (!threadMessages?.length) { + let threadMessages; + try { + threadMessages = await provider.getThreadMessages(message.threadId); + } catch (error) { + logger.error("Failed to fetch thread messages", { error }); + return; + } + if (!threadMessages?.length) { logger.error("No thread messages found, cannot proceed."); return; }apps/web/utils/assess.ts (1)
103-111: Avoid leaking provider internals via (client as any).client; extend the abstractionAccessing the internal Gmail client breaks encapsulation and makes this code provider-specific. Add explicit methods to EmailProvider (e.g., getFiltersCount, getForwardingAddressesCount) and implement them in concrete providers.
I can draft the interface/type changes in "@/utils/email/types" and provider implementations if you want.
Also applies to: 113-127
🧹 Nitpick comments (18)
apps/web/utils/assistant/process-assistant-email.ts (1)
268-279: Only the first label is applied; apply both “processing” and “assistant”You compute two labels but only apply labels[0]. Apply both as the comment suggests.
if (labels.length) { - // Fire and forget the initial labeling - provider.labelMessage(messageId, labels[0]).catch((error) => { - logger.error("Error labeling message", { error }); - }); + // Fire and forget applying both labels + for (const labelId of labels) { + provider.labelMessage(messageId, labelId).catch((error) => { + logger.error("Error labeling message", { error, labelId }); + }); + } }apps/web/utils/ai/actions.ts (1)
68-70: Improve unknown action error messageInterpolating the whole action object produces “[object Object]”. Log the action type or a serialized form for clarity.
- default: - throw new Error(`Unknown action: ${action}`); + default: + throw new Error(`Unknown action type: ${action.type}`);apps/web/app/api/outlook/webhook/process-history-item.ts (1)
335-338: Consistent error formatting in cleanup catchElsewhere in this file you use formatError(). Use the same here for consistency and cleaner logs.
- } catch (cleanupError) { - logger.error("Error during thread draft cleanup", { - ...loggerOptions, - error: cleanupError, - }); + } catch (cleanupError) { + logger.error("Error during thread draft cleanup", { + ...loggerOptions, + error: formatError(cleanupError), + }); }apps/web/utils/scheduled-actions/executor.ts (2)
121-133: Avoid string-matching provider errorsRelying on error.message text is brittle. Prefer a typed NotFoundError (or a discriminant) thrown by providers so you can detect it reliably.
I can help add a custom error type to the provider layer and switch this check to
instanceof NotFoundError. Want a patch?
81-88: Consider persisting error details on failureCurrently status is set to FAILED without storing diagnostic info. Persisting a truncated error message and timestamp can help ops.
apps/web/utils/cold-email/is-cold-email.ts (1)
207-221: LLM robustness: add retry/fallback if not already provided by createGenerateObjectPer our LLM guidelines, wrap the generateObject call with retry (e.g., withRetry) and consider a conservative default when the model response is invalid.
Does createGenerateObject already include retry/backoff? If not, I can add withRetry here. Let me know.
apps/web/utils/ai/choose-rule/draft-management.ts (1)
74-77: Avoid logging full draft contents; truncate to reduce PII riskCurrent logs include entire draft content. Truncate to a safe preview.
- logger.info("Comparing draft content", { - original: originalContent, - current: currentReplyContent, - }); + logger.info("Comparing draft content", { + original: originalContent?.slice(0, 200), + current: currentReplyContent.slice(0, 200), + });apps/web/utils/ai/choose-rule/run-rules.ts (1)
110-111: Remove empty conditional blockNo-op block can be removed to reduce noise.
- if (!isTest) { - } + // no-opapps/web/utils/assess.ts (2)
94-100: Avoid negative label counts; clamp at 0If a mailbox has fewer than default system labels, result could be negative.
- const DEFAULT_LABEL_COUNT = 13; - return labels.length - DEFAULT_LABEL_COUNT; + const DEFAULT_LABEL_COUNT = 13; + return Math.max(0, labels.length - DEFAULT_LABEL_COUNT); } else { - const DEFAULT_LABEL_COUNT = 8; - return labels.length - DEFAULT_LABEL_COUNT; + const DEFAULT_LABEL_COUNT = 8; + return Math.max(0, labels.length - DEFAULT_LABEL_COUNT);
52-60: DRY: unify unread count helpersgetUnreadEmailCount and getUnreadCount duplicate logic. Consolidate to one function and reuse.
Also applies to: 72-80
apps/web/app/api/watch/controller.ts (1)
4-4: Import path refactor looks correctType-only import from "@/utils/email/types" is consistent with the refactor.
Consider avoiding the string literal check on provider.name ("microsoft-entra-id"). If possible, centralize provider identifiers in the types module (e.g., a ProviderId union or constants) and compare against that to prevent drift:
- provider.name === "microsoft-entra-id" + provider.kind === ProviderId.Microsoftapps/web/utils/reply-tracker/draft-tracking.ts (1)
9-9: Centralized EmailProvider type import is correctChange is consistent with the refactor and uses a type-only import as recommended.
There’s duplication between the Gmail-based and provider-based functions. Consider consolidating around the provider-based APIs and delegating Gmail logic through the Gmail provider to reduce maintenance overhead.
apps/web/utils/reply-tracker/generate-draft.ts (1)
114-114: Nit: Document cache invalidation/TTL strategyGood to check Redis before drafting. Consider noting (or enforcing) TTL/eviction conditions so replies don’t go stale when knowledge base or writing style changes.
apps/web/__tests__/ai/reply/reply-context-collector.test.ts (1)
15-17: Clear mocks between tests — nice additionThis improves isolation. If you ever spy on built-ins or non-mocked modules, consider also calling vi.restoreAllMocks() to restore originals.
apps/web/utils/ai/choose-rule/choose-args.ts (1)
49-69: Use logger.with() locally to avoid repeating contextYou repeat email/threadId in multiple logs. Create a per-call logger via with() once and reuse. This follows our guidance to use .with() only inside functions.
Apply within this block:
- logger.info("Generating draft", { - email: emailAccount.email, - threadId: message.threadId, - }); + const log = logger.with({ + email: emailAccount.email, + threadId: message.threadId, + }); + log.info("Generating draft"); draft = await fetchMessagesAndGenerateDraft( emailAccount, message.threadId, client, ); - logger.info("Draft generated", { - email: emailAccount.email, - threadId: message.threadId, - }); + log.info("Draft generated"); } catch (error) { - logger.error("Failed to generate draft", { - email: emailAccount.email, - threadId: message.threadId, - error, - }); + log.error("Failed to generate draft", { error });apps/web/utils/ai/reply/reply-context-collector.ts (1)
153-156: Add context to trace and confirm PII-safe loggingGood move shifting verbose data to trace. Add user context for correlation, and ensure trace logs aren’t shipped in production.
- logger.trace("Finalizing results", { - notes: finalResult.notes, - relevantEmails: finalResult.relevantEmails, - }); + logger.trace("Finalizing results", { + email: emailAccount.email, + notes: finalResult.notes, + relevantEmails: finalResult.relevantEmails, + });Please confirm trace level is disabled or redacted in production deployments.
apps/web/utils/email/provider.ts (1)
12-15: Narrow provider type to prevent invalid inputsAvoid stringly-typed providers by constraining the union inline. Keeps typing tight without introducing a type alias.
export async function createEmailProvider({ emailAccountId, provider, }: { emailAccountId: string; - provider: string | null; + provider: "google" | "microsoft-entra-id" | null; }): Promise<EmailProvider> {apps/web/utils/email/google.ts (1)
399-405: Guideline deviation: avoid TypeScript enums
GmailLabelis an enum; project guidelines forbid enums.
Consider converting it to a string-literal union oras constobject map to align with the coding standards.
| async createLabel(name: string): Promise<EmailLabel> { | ||
| const label = await createGmailLabel({ | ||
| gmail: this.client, | ||
| name, | ||
| messageListVisibility: messageVisibility.show, | ||
| labelListVisibility: labelVisibility.labelShow, | ||
| }); | ||
|
|
There was a problem hiding this comment.
createLabel signature mismatch with interface
EmailProvider#createLabel declares two parameters.
Add the (unused) optional description parameter to keep the implementation compatible.
-async createLabel(name: string): Promise<EmailLabel> {
+async createLabel(name: string, _description?: string): Promise<EmailLabel> {📝 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 createLabel(name: string): Promise<EmailLabel> { | |
| const label = await createGmailLabel({ | |
| gmail: this.client, | |
| name, | |
| messageListVisibility: messageVisibility.show, | |
| labelListVisibility: labelVisibility.labelShow, | |
| }); | |
| async createLabel(name: string, _description?: string): Promise<EmailLabel> { | |
| const label = await createGmailLabel({ | |
| gmail: this.client, | |
| name, | |
| messageListVisibility: messageVisibility.show, | |
| labelListVisibility: labelVisibility.labelShow, | |
| }); |
🤖 Prompt for AI Agents
In apps/web/utils/email/google.ts around lines 301 to 308, the createLabel
method signature does not match the EmailProvider interface because it lacks the
optional description parameter. Add an optional description parameter to the
createLabel method signature, even if unused, to maintain compatibility with the
interface.
| async unwatchEmails(): Promise<void> { | ||
| await unwatchGmail(this.client); | ||
| } |
There was a problem hiding this comment.
unwatchEmails lacks the subscriptionId parameter
The interface defines unwatchEmails(subscriptionId?: string).
Even if Gmail ignores the argument, the method must accept it:
-async unwatchEmails(): Promise<void> {
+async unwatchEmails(_subscriptionId?: string): Promise<void> {📝 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 unwatchEmails(): Promise<void> { | |
| await unwatchGmail(this.client); | |
| } | |
| async unwatchEmails(_subscriptionId?: string): Promise<void> { | |
| await unwatchGmail(this.client); | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/email/google.ts around lines 657 to 659, the unwatchEmails
method is missing the subscriptionId parameter defined in the interface. Update
the method signature to accept an optional subscriptionId parameter
(subscriptionId?: string) even if it is not used inside the method, to maintain
interface compliance.
| async createLabel(name: string): Promise<EmailLabel> { | ||
| const label = await createOutlookLabel({ | ||
| client: this.client, | ||
| name, | ||
| }); | ||
|
|
||
| return { | ||
| id: label.id, | ||
| name: label.displayName || label.id, | ||
| type: "user", | ||
| }; |
There was a problem hiding this comment.
createLabel signature does not satisfy EmailProvider interface
EmailProvider#createLabel expects (name: string, description?: string).
This implementation omits the second parameter, causing TypeScript implementation errors.
-async createLabel(name: string): Promise<EmailLabel> {
+async createLabel(name: string, _description?: string): Promise<EmailLabel> {📝 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 createLabel(name: string): Promise<EmailLabel> { | |
| const label = await createOutlookLabel({ | |
| client: this.client, | |
| name, | |
| }); | |
| return { | |
| id: label.id, | |
| name: label.displayName || label.id, | |
| type: "user", | |
| }; | |
| async createLabel(name: string, _description?: string): Promise<EmailLabel> { | |
| const label = await createOutlookLabel({ | |
| client: this.client, | |
| name, | |
| }); | |
| return { | |
| id: label.id, | |
| name: label.displayName || label.id, | |
| type: "user", | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 286 to 296, the createLabel
method signature lacks the optional description parameter required by the
EmailProvider interface. Update the createLabel method to accept a second
optional parameter description of type string and ensure it is included in the
method signature to satisfy the interface contract.
| // Outlook attachments return the data directly, not base64 encoded | ||
| // We need to convert it to base64 for consistency with Gmail | ||
| const data = attachment.contentBytes | ||
| ? Buffer.from(attachment.contentBytes, "base64").toString("base64") | ||
| : ""; | ||
|
|
||
| return { | ||
| data, | ||
| size: attachment.size || 0, | ||
| }; |
There was a problem hiding this comment.
Incorrect attachment encoding logic
contentBytes are documented as raw bytes.
Passing "base64" as the input encoding to Buffer.from() assumes the data is already base-64, so the second .toString("base64") just round-trips or throws on non-base64 payloads.
-const data = attachment.contentBytes
- ? Buffer.from(attachment.contentBytes, "base64").toString("base64")
+const data = attachment.contentBytes
+ ? Buffer.from(attachment.contentBytes).toString("base64")
: "";📝 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.
| // Outlook attachments return the data directly, not base64 encoded | |
| // We need to convert it to base64 for consistency with Gmail | |
| const data = attachment.contentBytes | |
| ? Buffer.from(attachment.contentBytes, "base64").toString("base64") | |
| : ""; | |
| return { | |
| data, | |
| size: attachment.size || 0, | |
| }; | |
| // Outlook attachments return the data directly, not base64 encoded | |
| // We need to convert it to base64 for consistency with Gmail | |
| const data = attachment.contentBytes | |
| ? Buffer.from(attachment.contentBytes).toString("base64") | |
| : ""; | |
| return { | |
| data, | |
| size: attachment.size || 0, | |
| }; |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 508 to 517, the code
incorrectly treats contentBytes as base64-encoded by passing "base64" to
Buffer.from(), which expects a base64 string but contentBytes are raw bytes. To
fix this, remove the "base64" encoding argument from Buffer.from() so it
correctly interprets contentBytes as raw bytes, then convert the buffer to a
base64 string using .toString("base64") for consistent encoding.
| // Add folder filter based on type or labelId | ||
| if (query?.labelId) { | ||
| // Use labelId as parentFolderId (should be lowercase for Outlook) | ||
| filters.push(`parentFolderId eq '${query.labelId.toLowerCase()}'`); | ||
| } else if (query?.type === "all") { | ||
| // For "all" type, include both inbox and archive | ||
| filters.push( | ||
| "(parentFolderId eq 'inbox' or parentFolderId eq 'archive')", | ||
| ); | ||
| } else { | ||
| // Default to inbox only | ||
| filters.push("parentFolderId eq 'inbox'"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Folder filter uses literal names, not Microsoft Graph folder IDs
parentFolderId must be a GUID. Hard-coding 'inbox' / 'archive' will never match and silently return an empty result set.
Retrieve folder IDs first (getFolderIds) and plug those values into the filter.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 535 to 548, the folder filter
uses literal folder names like 'inbox' and 'archive' instead of the required
Microsoft Graph folder GUIDs, causing no results to be returned. To fix this,
first retrieve the actual folder IDs using the getFolderIds function, then
replace the hardcoded folder names in the filter with these retrieved GUIDs to
ensure the filter matches correctly.
| isReplyInThread(message: ParsedMessage): boolean { | ||
| try { | ||
| return atob(message.conversationIndex || "").length > 22; | ||
| } catch (error) { | ||
| logger.warn("Invalid conversationIndex base64", { | ||
| conversationIndex: message.conversationIndex, | ||
| error, | ||
| }); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
atob is not available in Node runtimes
atob is a browser-only global; backend execution will crash with ReferenceError.
Use Buffer.from(str, 'base64') instead:
-return atob(message.conversationIndex || "").length > 22;
+return Buffer.from(message.conversationIndex ?? "", "base64").length > 22;📝 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.
| isReplyInThread(message: ParsedMessage): boolean { | |
| try { | |
| return atob(message.conversationIndex || "").length > 22; | |
| } catch (error) { | |
| logger.warn("Invalid conversationIndex base64", { | |
| conversationIndex: message.conversationIndex, | |
| error, | |
| }); | |
| return false; | |
| } | |
| } | |
| isReplyInThread(message: ParsedMessage): boolean { | |
| try { | |
| return Buffer.from(message.conversationIndex ?? "", "base64").length > 22; | |
| } catch (error) { | |
| logger.warn("Invalid conversationIndex base64", { | |
| conversationIndex: message.conversationIndex, | |
| error, | |
| }); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 811 to 821, the use of the
browser-only global function atob causes a ReferenceError in Node.js
environments. Replace atob(message.conversationIndex || "") with
Buffer.from(message.conversationIndex || "", "base64").toString("binary") to
decode the base64 string safely in Node.js. Adjust the length check accordingly
to maintain the original logic.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (18)
apps/web/app/api/watch/controller.ts (1)
66-73: Bug: early return skips DB cleanup on invalid_grantThe
returnon invalid_grant prevents clearingwatchEmails*fields, contradicting the comment “Clear the watch data regardless of provider”. Remove the early return (or move cleanup to a finally block) so DB cleanup always runs.Apply this diff:
- if (error instanceof Error && error.message.includes("invalid_grant")) { - logger.warn("Error unwatching emails, invalid grant", { - emailAccountId, - providerName: provider.name, - }); - return; - } + if (error instanceof Error && error.message.includes("invalid_grant")) { + logger.warn("Error unwatching emails, invalid grant", { + emailAccountId, + providerName: provider.name, + }); + // Proceed to cleanup below + }Also applies to: 82-90
apps/web/utils/reply-tracker/check-sender-reply-history.ts (3)
21-25: Default behavior contradicts the comment (assumes “replied” on invalid email)Comment says “assume a reply might be needed,” but code returns
hasReplied: true. That skips reply checks. Flip tofalseto match intent.- // Default to assuming a reply might be needed if email is invalid - return { hasReplied: true, receivedCount: 0 }; + // Default to assuming a reply might be needed if email is invalid + return { hasReplied: false, receivedCount: 0 };
27-38: Handle partial failures with Promise.allSettled and remove redundant catchCatching the entire
Promise.allhides which call failed and forces a single default for both results. UsePromise.allSettledto preserve successful values and default only the failed ones.- // Run checks in parallel for efficiency - const [hasReplied, receivedCount] = await Promise.all([ - emailProvider.checkIfReplySent(cleanSenderEmail), - emailProvider.countReceivedMessages(cleanSenderEmail, receivedThreshold), - ]).catch((error) => { - logger.error("Timeout or error in parallel operations", { - error, - cleanSenderEmail, - }); - return [true, 0] as const; // Safe defaults - }); + // Run checks in parallel for efficiency and handle partial failures + const [repliedRes, receivedRes] = await Promise.allSettled([ + emailProvider.checkIfReplySent(cleanSenderEmail), + emailProvider.countReceivedMessages(cleanSenderEmail, receivedThreshold), + ]); + const hasReplied = + repliedRes.status === "fulfilled" ? repliedRes.value : false; + const receivedCount = + receivedRes.status === "fulfilled" ? receivedRes.value : 0;
49-56: Same default mismatch on catch: align with “might need reply” intentOn overall error, you again return
hasReplied: true. Align with the stated intent.- // Default to assuming a reply might be needed on error - return { hasReplied: true, receivedCount: 0 }; + // Default to assuming a reply might be needed on error + return { hasReplied: false, receivedCount: 0 };apps/web/utils/assistant/process-assistant-email.ts (2)
92-94: Guard when In-Reply-To header is missing
getOriginalMessageis called with a possibly undefined id. Avoid provider errors by guarding.- const originalMessageId = firstMessageToAssistant.headers["in-reply-to"]; - const originalMessage = await provider.getOriginalMessage(originalMessageId); + const originalMessageId = firstMessageToAssistant.headers["in-reply-to"]; + const originalMessage = originalMessageId + ? await provider.getOriginalMessage(originalMessageId) + : null;
274-279: Only one label is applied despite doc stating both should be addedDocstring says “label the message with processing and assistant labels,” but only
labels[0]is applied. Apply both (fire-and-forget remains).- if (labels.length) { - // Fire and forget the initial labeling - provider.labelMessage(messageId, labels[0]).catch((error) => { - logger.error("Error labeling message", { error }); - }); - } + if (labels.length) { + // Fire and forget the initial labeling for both labels + for (const id of labels) { + provider.labelMessage(messageId, id).catch((error) => { + logger.error("Error labeling message", { error, labelId: id }); + }); + } + }apps/web/utils/scheduled-actions/executor.ts (1)
121-130: Standardize “not found” handling across providersCatching a Gmail-specific message (
"Requested entity was not found.") couples this to one provider. Push the normalization to the provider layer: either havegetMessagereturnnullfor 404s or throw a standardizedNotFoundError.Proposed shape (provider layer):
getMessage(id): Promise<EmailForAction | null>returnsnullwhen not found; or- throw
new NotFoundError("message", id)and export its type.Then simplify this function to:
- } catch (error: unknown) { - if ( - error instanceof Error && - error.message === "Requested entity was not found." - ) { - logger.info("Email not found during validation", { - messageId: scheduledAction.messageId, - scheduledActionId: scheduledAction.id, - }); - return null; - } - throw error; - } + } catch (error: unknown) { + // If using a standardized NotFoundError: + if (error && (error as any).name === "NotFoundError") { + logger.info("Email not found during validation", { + messageId: scheduledAction.messageId, + scheduledActionId: scheduledAction.id, + }); + return null; + } + throw error; + }I can add a small
NotFoundErrorclass and plumb it through the providers if helpful.apps/web/utils/gmail/message.ts (1)
213-227: PUBLIC_DOMAINS contains entries with leading '@' — matching will failextractDomainFromEmail returns domains like "me.com"/"hey.com". Entries "@me.com" and "@hey.com" will never match.
Apply this diff:
const PUBLIC_DOMAINS = new Set([ "gmail.com", "yahoo.com", "hotmail.com", "outlook.com", "aol.com", "icloud.com", - "@me.com", + "me.com", "protonmail.com", "zoho.com", "yandex.com", "fastmail.com", "gmx.com", - "@hey.com", + "hey.com", ]);apps/web/utils/ai/choose-rule/match-rules.ts (2)
380-397: Guard against undefined extracted email before calling startsWithextractEmailAddress can return undefined/null for malformed headers. Current code may throw.
- const extractedSenderEmail = extractEmailAddress(senderEmail); + const extractedSenderEmail = extractEmailAddress(senderEmail); - if ( - noReplyPrefixes.some((prefix) => extractedSenderEmail.startsWith(prefix)) - ) { + if (!extractedSenderEmail) { + return potentialMatches; + } + if (noReplyPrefixes.some((prefix) => extractedSenderEmail.startsWith(prefix))) { return potentialMatches; }
305-314: Harden regex tests against missing headersIf a message lacks from/to/subject, safeRegexTest receives undefined and RegExp.test will throw. Default to empty strings.
- const fromMatch = from - ? safeRegexTest(from, message.headers.from, true) - : true; - const toMatch = to ? safeRegexTest(to, message.headers.to, true) : true; - const subjectMatch = subject - ? safeRegexTest(subject, message.headers.subject, false) - : true; + const fromMatch = from + ? safeRegexTest(from, message.headers.from || "", true) + : true; + const toMatch = to + ? safeRegexTest(to, message.headers.to || "", true) + : true; + const subjectMatch = subject + ? safeRegexTest(subject, message.headers.subject || "", false) + : true;apps/web/utils/cold-email/is-cold-email.ts (4)
162-171: Likely Prisma misuse: findUnique cannot include non-unique filtersfindUnique expects only a unique selector. Including status will either type-error or be ignored. Use findFirst with a compound where.
- const coldEmail = await prisma.coldEmail.findUnique({ - where: { - emailAccountId_fromEmail: { - emailAccountId, - fromEmail: from, - }, - status: ColdEmailStatus.AI_LABELED_COLD, - }, - select: { id: true }, - }); + const coldEmail = await prisma.coldEmail.findFirst({ + where: { + emailAccountId, + fromEmail: from, + status: ColdEmailStatus.AI_LABELED_COLD, + }, + select: { id: true }, + });
74-86: Add fallback for AI failures to avoid blocking flowIf the LLM call fails, the whole check throws. Catch and default to non-cold with an AI reason.
- const res = await aiIsColdEmail(email, emailAccount, modelType); + let res: { coldEmail: boolean; reason: string } | undefined; + try { + const aiRes = await aiIsColdEmail(email, emailAccount, modelType); + res = aiRes; + } catch (error) { + logger.error("AI cold email check failed", { ...loggerOptions, error }); + return { isColdEmail: false, reason: "ai", aiReason: null }; + } logger.info("AI is cold email?", { ...loggerOptions, - coldEmail: res.coldEmail, + coldEmail: res?.coldEmail, }); return { - isColdEmail: !!res.coldEmail, + isColdEmail: !!res?.coldEmail, reason: "ai", - aiReason: res.reason, + aiReason: res?.reason, };
140-153: Mirror AI fallback in provider-based pathEnsure consistent behavior when using EmailProvider variant.
- const res = await aiIsColdEmail(email, emailAccount, modelType); + let res: { coldEmail: boolean; reason: string } | undefined; + try { + const aiRes = await aiIsColdEmail(email, emailAccount, modelType); + res = aiRes; + } catch (error) { + logger.error("AI cold email check failed", { ...loggerOptions, error }); + return { isColdEmail: false, reason: "ai", aiReason: null }; + } logger.info("AI is cold email?", { ...loggerOptions, - coldEmail: res.coldEmail, + coldEmail: res?.coldEmail, }); return { - isColdEmail: !!res.coldEmail, + isColdEmail: !!res?.coldEmail, reason: "ai", - aiReason: res.reason, + aiReason: res?.reason, };
281-316: Wrap provider side-effects to avoid failing after DB writeLabel/archive/markRead can fail; wrap to prevent partial failures from crashing and to log context.
- if (coldEmailLabel?.name) { - await provider.labelMessage(email.id, coldEmailLabel.name); - } - - // For archiving and marking as read, we'll need to implement these in the provider - if (shouldArchive) { - await provider.archiveThread(email.threadId, emailAccount.email); - } - - if (shouldMarkRead) { - await provider.markReadThread(email.threadId, true); - } + try { + if (coldEmailLabel?.name) { + await provider.labelMessage(email.id, coldEmailLabel.name); + } + if (shouldArchive) { + await provider.archiveThread(email.threadId, emailAccount.email); + } + if (shouldMarkRead) { + await provider.markReadThread(email.threadId, true); + } + } catch (error) { + logger.error("Failed to apply cold email actions", { + emailId: email.id, + threadId: email.threadId, + userEmail: emailAccount.email, + error, + }); + }apps/web/utils/assess.ts (2)
104-111: Avoid reaching into provider internals and relying on instanceofAccessing (client as any).client and branching on Gmail ties this util to concrete providers. Prefer exposing capabilities on EmailProvider (e.g., listFilters, listForwardingAddresses) and using them here.
Proposed shape:
- Extend EmailProvider with optional:
- listFilters(): Promise<unknown[]>
- listForwardingAddresses(): Promise<unknown[]>
Then simplify:
- if (client instanceof GmailProvider) { - const gmail = (client as any).client; - const filters = await getFilters(gmail); - return filters.length; - } - return 0; + const filters = await client.listFilters?.(); + return Array.isArray(filters) ? filters.length : 0;- if (client instanceof GmailProvider) { - try { - const gmail = (client as any).client; - const forwardingAddresses = await getForwardingAddresses(gmail); - return forwardingAddresses.length; - } catch (error) { - logger.error("Error getting forwarding addresses", { error }); - return 0; - } - } - return 0; + try { + const forwardingAddresses = await client.listForwardingAddresses?.(); + return Array.isArray(forwardingAddresses) ? forwardingAddresses.length : 0; + } catch (error) { + logger.error("Error getting forwarding addresses", { error }); + return 0; + }Also applies to: 114-127
14-21: Deduplicate unread count helpersTo avoid maintaining two identical implementations, replace
getUnreadEmailCountwith the already-exportedgetUnreadCountand remove the redundant helper.• apps/web/utils/assess.ts, line 16:
- const unreadCount = await getUnreadEmailCount(client); + const unreadCount = await getUnreadCount(client);• apps/web/utils/assess.ts, lines 52–61: remove the entire
getUnreadEmailCountfunction-async function getUnreadEmailCount(client: EmailProvider) { - if (client instanceof GmailProvider) { - const label = await client.getLabelById(GmailLabel.UNREAD); - return label?.threadsTotal || 0; - } else { - const label = await client.getLabelById(OutlookLabel.UNREAD); - return label?.threadsTotal || 0; - } -}apps/web/utils/reply-tracker/generate-draft.ts (2)
45-45: Avoid logging full draft content (PII leakage risk)Logging the entire generated draft at info level can leak sensitive user data to logs. Log metadata (e.g., length or a hash) instead.
Apply this diff to redact content:
- logger.info("Draft generated", { result }); + logger.info("Draft generated", { + contentLength: typeof result === "string" ? result.length : 0, + });
51-55: Harden external call: wrap draft creation in try/catchEmail provider calls can fail; add error handling with structured logging.
- // 2. Create draft - await client.draftEmail(message, { content: result }); - - logger.info("Draft created"); + // 2. Create draft + try { + await client.draftEmail(message, { content: result }); + logger.info("Draft created"); + } catch (error) { + const errorMessage = error instanceof Error ? error.message : String(error); + const errorStack = error instanceof Error ? error.stack : undefined; + logger.error("Failed to create draft", { + emailAccountId, + messageId: message.id, + threadId: message.threadId, + errorMessage, + errorStack, + }); + throw error; + }
♻️ Duplicate comments (1)
apps/web/app/api/messages/route.ts (1)
86-102: ConfirmwithEmailProviderprovides auth contextSame note as in messages/batch: ensure
withEmailProviderenforces authentication/authorization and email-account ownership.If not already verified, run the earlier script to inspect the middleware and legacy imports.
🧹 Nitpick comments (23)
apps/web/app/api/messages/route.ts (1)
53-68: Move provider-specific filtering out of the routeThis block embeds Gmail/Outlook specifics in the API route. Per the team's pattern to isolate provider logic (apps/web/utils/gmail/**), consider moving this into provider-level helpers (e.g.,
emailProvider.filterIncomingMessages(...)) to keep routes provider-agnostic.apps/web/utils/ai/choose-rule/execute.ts (1)
45-50: Nit: variable naming clarity
pendingRulesholds the result ofupdateMany(with acount). Consider renaming toupdatedorupdateResultfor readability.- const pendingRules = await prisma.executedRule.updateMany({ + const updateResult = await prisma.executedRule.updateMany({ where: { id: executedRule.id, status: ExecutedRuleStatus.PENDING }, data: { status: ExecutedRuleStatus.APPLYING }, }); - if (pendingRules.count === 0) { + if (updateResult.count === 0) { logger.info("Executed rule is not pending or does not exist"); return; }apps/web/app/api/outlook/webhook/process-history-item.ts (1)
335-338: Consistent error formatting in cleanup catchElsewhere you use
formatError(...); here rawcleanupErroris logged. Consider normalizing for consistency and safer serialization.- logger.error("Error during thread draft cleanup", { - ...loggerOptions, - error: cleanupError, - }); + logger.error("Error during thread draft cleanup", { + ...loggerOptions, + error: formatError(cleanupError), + });apps/web/utils/categorize/senders/categorize.ts (1)
19-37: Consider moving Gmail-specific categorizeSender to gmail/ utils or deprecate in favor of provider-based APIThis function is Gmail-specific (uses gmail_v1 and gmail thread helpers). To keep provider-specific details isolated, either move it under
apps/web/utils/gmail/or steer callers tocategorizeSenderWithProviderbelow and deprecate this one.apps/web/utils/ai/actions.ts (1)
69-70: Improve unknown action error messageUse
action.typeto avoid logging[object Object]and to keep errors clear.- default: - throw new Error(`Unknown action: ${action}`); + default: + throw new Error(`Unknown action type: ${action.type}`);apps/web/utils/ai/choose-rule/run-rules.ts (1)
110-112: Remove empty if blockThis no-op block adds noise.
- if (!isTest) { - } + // no-opapps/web/utils/scheduled-actions/executor.ts (1)
238-244: Consider persisting failure details for diagnosticsIf schema allows, store an error summary (e.g.,
failureReason) when marking FAILED. This speeds up ops/debugging.await prisma.scheduledAction.update({ where: { id: scheduledActionId }, data: { status: ScheduledActionStatus.FAILED, + // failureReason: error instanceof Error ? error.message : String(error), }, });apps/web/utils/reply-tracker/outbound.ts (1)
101-186: Reduce duplication and isolate provider-specific logicThis file maintains both Gmail-only and provider-agnostic flows. To align with the “keep provider-specific logic in utils/gmail” learning and reduce duplication:
- Move
handleOutboundReply(Gmail path) toutils/gmail/….- Keep only the provider-agnostic
handleOutboundReplyWithProviderhere.- Or have the Gmail path call the provider-based function by constructing a Gmail-backed provider.
I can prepare a refactor patch that consolidates to the provider-based path and relocates Gmail-specific code.
apps/web/utils/gmail/message.ts (1)
201-205: Prefer integer epoch seconds for clarityThe Date constructor will coerce, but using getTime() and Math.floor makes intent explicit and avoids fractional seconds.
- const previousEmails = await findPreviousEmailsWithSender(client, { - sender: options.from, - dateInSeconds: +new Date(options.date) / 1000, - }); + const previousEmails = await findPreviousEmailsWithSender(client, { + sender: options.from, + dateInSeconds: Math.floor(options.date.getTime() / 1000), + });apps/web/utils/reply-tracker/draft-tracking.ts (3)
98-106: Guard similarity calc against undefined message textsentTextPlain can be undefined; ensure a string is passed.
- const similarityScore = calculateSimilarity( - executedAction.content, - sentTextPlain, - ); + const similarityScore = calculateSimilarity( + executedAction.content, + sentTextPlain || "", + );
212-220: Guard similarity calc against undefined message text (provider path)Mirror the guard in the provider-based function.
- const similarityScore = calculateSimilarity( - executedAction.content, - sentTextPlain, - ); + const similarityScore = calculateSimilarity( + executedAction.content, + sentTextPlain || "", + );
130-241: Reduce duplication between Gmail and provider variantstrackSentDraftStatus and trackSentDraftStatusWithProvider, and the cleanup counterparts, duplicate most logic. Consider a single implementation parameterized by { getDraft, deleteDraft } strategy to improve maintainability.
Also applies to: 366-483
apps/web/utils/assess.ts (1)
93-101: Clamp label count to non-negativeSubtracting DEFAULT_LABEL_COUNT can yield negative values. Clamp to zero.
- if (client instanceof GmailProvider) { - const DEFAULT_LABEL_COUNT = 13; - return labels.length - DEFAULT_LABEL_COUNT; - } else { - const DEFAULT_LABEL_COUNT = 8; - return labels.length - DEFAULT_LABEL_COUNT; - } + if (client instanceof GmailProvider) { + const DEFAULT_LABEL_COUNT = 13; + return Math.max(0, labels.length - DEFAULT_LABEL_COUNT); + } else { + const DEFAULT_LABEL_COUNT = 8; + return Math.max(0, labels.length - DEFAULT_LABEL_COUNT); + }apps/web/utils/reply-tracker/generate-draft.ts (2)
114-121: Cache read is well-placed; consider TTL/invalidations to prevent stale draftsReading Redis before heavy AI work is good. Consider adding TTL to saved replies and/or invalidating on events that change output (e.g., writing style updates or knowledge base changes) to avoid serving stale drafts.
16-16: Avoid logger shadowing; reuse a base logger with contextual .with()You create a global logger and then re-declare logger inside the function. Prefer a base logger at module scope and derive contextual loggers inside functions for clarity.
-const logger = createScopedLogger("generate-reply"); +const baseLogger = createScopedLogger("generate-reply"); @@ - const logger = createScopedLogger("generate-reply").with({ + const logger = baseLogger.with({ emailAccountId, messageId: message.id, threadId: message.threadId, });Also applies to: 27-31
apps/web/__tests__/ai/reply/reply-context-collector.test.ts (1)
89-93: Prefer console.debug for AI test diagnosticsProject guidelines recommend descriptive console.debug statements for AI tests. Consider switching all console.log occurrences in this file to console.debug.
Example diff (apply similarly to other occurrences):
- console.log( + console.debug( `Basic: LLM issued ${observedQueries.length} search call(s):`, observedQueries, );apps/web/utils/ai/choose-rule/choose-args.ts (1)
65-69: Log structured error details (message/stack) rather than raw Error objectSome log sinks don’t serialize Error objects well. Include message and stack explicitly.
- logger.error("Failed to generate draft", { - email: emailAccount.email, - threadId: message.threadId, - error, - }); + const errorMessage = error instanceof Error ? error.message : String(error); + const errorStack = error instanceof Error ? error.stack : undefined; + logger.error("Failed to generate draft", { + email: emailAccount.email, + threadId: message.threadId, + errorMessage, + errorStack, + });apps/web/utils/ai/reply/reply-context-collector.ts (1)
132-134: Lower verbosity for frequent search logsPer request volume, consider debug level to reduce noise while retaining observability.
- logger.info("Found emails", { emails: emails.length }); + logger.debug("Found emails", { count: emails.length }); // logger.trace("Found emails", { emails });apps/web/utils/email/provider.ts (1)
12-15: Narrow provider type for stronger type safetyConstrain provider to known literals to catch unsupported values at compile time.
-export async function createEmailProvider({ +type ProviderId = "google" | "microsoft-entra-id"; + +export async function createEmailProvider({ emailAccountId, provider, }: { emailAccountId: string; - provider: string | null; + provider: ProviderId | null; }): Promise<EmailProvider> {apps/web/utils/email/google.ts (2)
82-91: Avoid N + 1 calls – use the existing batch helper
getThreadsfetches a thread list then loops over each thread withgetThread, causing one API round-trip per thread.
You already importgetThreadsBatchfurther down – using it here would cut latency and Gmail quota dramatically.-const threadPromises = threads.map((thread) => this.getThread(thread.id!)); -return Promise.all(threadPromises); +const threadIds = threads.map((t) => t.id!).filter(Boolean); +return getThreadsBatch(threadIds, getAccessTokenFromClient(this.client));
272-279: Two public helpers for the same concern
markReadis a thin wrapper aroundgmailMarkReadThread(..., read: true), whilemarkReadThreadexposes the full boolean flag.
Keeping both invites confusion; consider removingmarkReadand rely on the more generalmarkReadThread.apps/web/utils/email/microsoft.ts (2)
502-512: Redundant base64 dance on attachments
attachment.contentBytesis already base64-encoded.
Decoding it into a buffer and immediately encoding again just allocates memory without transformation.-const data = attachment.contentBytes - ? Buffer.from(attachment.contentBytes, "base64").toString("base64") - : ""; +const data = attachment.contentBytes ?? "";
811-820: PreferBuffer.from(..., 'base64')overatobin Node
atobisn’t available in some Node runtimes and swallows non-ASCII silently.
UseBuffer.byteLength(Buffer.from(..., 'base64'))or similar to compute length and avoid try/catch noise.
| import { createScopedLogger } from "@/utils/logger"; | ||
| import { isIgnoredSender } from "@/utils/filter-ignored-senders"; | ||
| import type { EmailProvider } from "@/utils/email/provider"; | ||
| import type { EmailProvider } from "@/utils/email/types"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM: Updated type import aligns with modularized email provider types
Importing EmailProvider from "@/utils/email/types" (type-only) is correct and consistent.
Run this to verify no lingering imports from the old path remain:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Find any remaining imports from the old provider path across the repo
rg -n $'from\\s+[\'"]@/utils/email/provider[\'"]' -S || echo "No stale imports found ✔️"
# Optional: ensure all imports use type-only where appropriate
rg -n $'import\\s+\\{?\\s*EmailProvider\\s*\\}?\\s+from\\s+[\'"]@/utils/email/types[\'"]' -SLength of output: 2468
Fix stale EmailProvider type imports
The repository still contains type-only imports of EmailProvider from the old provider path. Please update them to the new types location. For example:
• apps/web/app/api/user/stats/newsletters/helpers.ts (line 1)
- import type { EmailProvider, EmailFilter } from "@/utils/email/provider";
+ import type { EmailProvider, EmailFilter } from "@/utils/email/types";After applying this, re-run your grep command to ensure no remaining imports from "@/utils/email/provider" reference EmailProvider.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/api/threads/route.ts at line 10, the import of the EmailProvider
type is from the outdated path "@/utils/email/types". Update this import to the
new correct path where EmailProvider is currently defined, matching the updated
project structure. After making this change, run a grep search to confirm no
other imports of EmailProvider remain from the old provider path
"@/utils/email/provider".
| logger.trace("Finalizing results", { | ||
| notes: finalResult.notes, | ||
| relevantEmails: finalResult.relevantEmails, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging full email contents; log metadata instead
Even at trace level, logging full email bodies risks PII exposure. Log lengths/counts instead.
- logger.trace("Finalizing results", {
- notes: finalResult.notes,
- relevantEmails: finalResult.relevantEmails,
- });
+ logger.debug("Finalizing results meta", {
+ notesLength: finalResult.notes?.length ?? 0,
+ relevantEmailsCount: finalResult.relevantEmails.length,
+ });📝 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.
| logger.trace("Finalizing results", { | |
| notes: finalResult.notes, | |
| relevantEmails: finalResult.relevantEmails, | |
| }); | |
| logger.debug("Finalizing results meta", { | |
| notesLength: finalResult.notes?.length ?? 0, | |
| relevantEmailsCount: finalResult.relevantEmails.length, | |
| }); |
🤖 Prompt for AI Agents
In apps/web/utils/ai/reply/reply-context-collector.ts around lines 159 to 162,
the current trace log outputs full email contents which risks exposing PII.
Modify the logger.trace call to log only metadata such as the count of relevant
emails and the length of notes instead of their full content. This ensures
sensitive information is not exposed while retaining useful debugging data.
| async getPreviousConversationMessages( | ||
| messageIds: string[], | ||
| ): Promise<ParsedMessage[]> { | ||
| return this.getThreadMessages(messageIds[0]); | ||
| } |
There was a problem hiding this comment.
getPreviousConversationMessages returns only one thread
Current implementation fetches the messages of messageIds[0] and ignores the rest.
Either accept a single threadId or iterate all IDs – as written it misleads the caller.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 275 to 279, the method
getPreviousConversationMessages accepts an array of messageIds but only fetches
messages for the first ID, ignoring the rest. To fix this, either change the
method signature to accept a single threadId if only one is intended, or update
the implementation to iterate over all messageIds, fetching and aggregating
messages for each ID before returning the combined result.
| // Add folder filter based on type or labelId | ||
| if (query?.labelId) { | ||
| // Use labelId as parentFolderId (should be lowercase for Outlook) | ||
| filters.push(`parentFolderId eq '${query.labelId.toLowerCase()}'`); | ||
| } else if (query?.type === "all") { | ||
| // For "all" type, include both inbox and archive | ||
| filters.push( | ||
| "(parentFolderId eq 'inbox' or parentFolderId eq 'archive')", | ||
| ); | ||
| } else { | ||
| // Default to inbox only | ||
| filters.push("parentFolderId eq 'inbox'"); | ||
| } |
There was a problem hiding this comment.
Folder filter uses literal names instead of folder IDs
parentFolderId eq 'inbox' / 'archive' will never match – Graph expects the GUID of the folder, not the well-known name.
Fetch the IDs via getFolderIds (you already do this elsewhere) and interpolate them here, otherwise every inbox query returns zero rows.
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 535 to 547, the folder filter
uses literal folder names like 'inbox' and 'archive' instead of their
corresponding folder IDs, causing queries to return no results. To fix this,
retrieve the actual folder IDs using the existing getFolderIds function and
replace the hardcoded folder names in the filter with these IDs. This ensures
the filter queries the correct folders by their GUIDs as required by Graph.
| createFilter(options: { | ||
| from: string; | ||
| addLabelIds?: string[]; | ||
| removeLabelIds?: string[]; | ||
| }): Promise<any>; | ||
| deleteFilter(id: string): Promise<any>; | ||
| createAutoArchiveFilter(options: { | ||
| from: string; | ||
| gmailLabelId?: string; | ||
| labelName?: string; | ||
| }): Promise<any>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any return types with explicit shapes
createFilter, deleteFilter, and createAutoArchiveFilter all promise any.
This breaks the “no implicit any / strict-null-checks” posture and forces every caller to cast or duck-type the result.
Define dedicated response types (even a minimal EmailFilterResponse interface) and use them here – the concrete providers already know the shape coming back from Gmail/Graph.
🤖 Prompt for AI Agents
In apps/web/utils/email/types.ts between lines 91 and 101, the methods
createFilter, deleteFilter, and createAutoArchiveFilter currently return
Promise<any>, which undermines type safety. Define explicit response interfaces,
such as EmailFilterResponse, that describe the expected structure of the
returned data from these methods. Then update the method signatures to return
Promise<EmailFilterResponse> or the appropriate specific type instead of
Promise<any> to enforce strict typing and improve code reliability.
1 participant
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests