Conversation
|
@edulelis is attempting to deploy a commit to the Inbox Zero OSS Program Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds MOVE_FOLDER action type and folderName field across schema, APIs, UI, validation, and action execution. Implements provider abstraction with Gmail and Outlook providers; Outlook supports moving threads to folders while Gmail logs unsupported. Updates auth token saving flow and introduces provider guards in certain routes. Adjusts tests and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant API as Rules API
participant Validator
participant DB
participant Runner as Action Runner
participant Provider as EmailProvider
User->>WebUI: Select MOVE_FOLDER + folderName
WebUI->>API: Create/Update Rule (folderName)
API->>Validator: Validate action (MOVE_FOLDER requires folderName)
Validator-->>API: Valid
API->>DB: Persist action { type: MOVE_FOLDER, folderName }
DB-->>API: OK
Note over Runner,Provider: On rule execution
Runner->>DB: Load rule actions
Runner->>Provider: moveThreadToFolder(threadId, userEmail, folderName)
alt Microsoft
Provider-->>Runner: Thread moved
else Google
Provider-->>Runner: No-op (warn)
end
Runner-->>User: Execution complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🔭 Outside diff range comments (18)
apps/web/utils/llms/model.ts (1)
139-151: Avoid non-null assertions (!) for Bedrock credentials; re-check inside credentialProvider or capture narrowed localsNon-null assertions violate our TS guidelines. Re-check within the async credentialProvider to satisfy the type system without using !.
- model: createAmazonBedrock({ + model: createAmazonBedrock({ // accessKeyId: env.BEDROCK_ACCESS_KEY, // secretAccessKey: env.BEDROCK_SECRET_KEY, // sessionToken: undefined, region: env.BEDROCK_REGION, - credentialProvider: async () => ({ - accessKeyId: env.BEDROCK_ACCESS_KEY!, - secretAccessKey: env.BEDROCK_SECRET_KEY!, - sessionToken: undefined, - }), + credentialProvider: async () => { + if (!env.BEDROCK_ACCESS_KEY || !env.BEDROCK_SECRET_KEY) { + throw new Error("Missing AWS Bedrock credentials"); + } + return { + accessKeyId: env.BEDROCK_ACCESS_KEY, + secretAccessKey: env.BEDROCK_SECRET_KEY, + sessionToken: undefined, + }; + }, })(modelName),apps/web/utils/ai/assistant/process-user-request.ts (2)
158-159: Avoid logging full LLM inputs (risk of PII leakage); log metadata insteadLogging allMessages can expose email content. Prefer logging only metadata (e.g., counts, roles) and omit bodies/headers.
- logger.trace("Input", { allMessages }); + logger.trace("Input", { + messageCount: allMessages.length, + roles: allMessages.map((m) => m.role), + hasOriginalEmail: Boolean(originalEmail), + hasMatchedRule: Boolean(matchedRule), + });
204-215: Add retry wrapper around the LLM call in process-user-request.tsThe
createGenerateTexthelper only falls back to a backup model once—it doesn’t retry the primary model on transient failures. To guard against intermittent API errors, wrap yourgenerateTextinvocation withwithRetryfrom"@/utils/llms".• File: apps/web/utils/ai/assistant/process-user-request.ts
– At the top, update the import to includewithRetry:
ts import { createGenerateText, withRetry } from "@/utils/llms";
– Around line 210, change your call from:
diff - const result = await generateText({ - ...modelOptions, - messages: allMessages, - stopWhen: stepCountIs(5), - tools: { update_conditional_operator: tool({ /* … */ }) }, - }); + const result = await withRetry( + () => generateText({ + ...modelOptions, + messages: allMessages, + stopWhen: stepCountIs(5), + tools: { update_conditional_operator: tool({ /* … */ }) }, + }), + { retries: 2, retryDelay: 1000 } + );
Adjustretries/retryDelayto match project conventions.apps/web/components/editor/extensions/LabelMention.tsx (3)
90-92: Fix strictNullChecks: possible use-before-assign for component/popupUnder strict null checks, these locals should be nullable to avoid “used before assignment” and to make the guard checks meaningful.
- let component: ReactRenderer<MentionListRef>; - let popup: HTMLElement; + let component: ReactRenderer<MentionListRef> | null = null; + let popup: HTMLElement | null = null; @@ - popup.parentNode.removeChild(popup); + popup.parentNode.removeChild(popup); @@ - if (component) { - component.destroy(); - } + if (component) { + component.destroy(); + component = null; + }Also applies to: 135-141
103-105: Replace console. with project logger (console disallowed)*Guidelines forbid console.* in TS/TSX. Replace with createScopedLogger and keep messages at the same levels.
+import { createScopedLogger } from "@/utils/logger"; +const logger = createScopedLogger("editor/LabelMention"); @@ - // Silently handle cleanup errors to prevent crashes - console.warn("Error during mention cleanup:", error); + // Silently handle cleanup errors to prevent crashes + logger.warn("Error during mention cleanup", { error }); @@ - console.error("Error during mention start:", error); + logger.error("Error during mention start", { error }); @@ - console.error("Error during mention update:", error); + logger.error("Error during mention update", { error }); @@ - console.error("Error during mention keydown:", error); + logger.error("Error during mention keydown", { error });Also applies to: 126-129, 151-154, 165-169
277-287: Escape attribute values in custom HTML renderer (prevent XSS)Interpolating id/label directly into HTML can allow attribute injection if label contains quotes or angle brackets. Escape attributes or build via DOM.
- markdownIt.renderer.rules.mention_open = ( + const escapeAttr = (val: string) => + val + .replace(/&/g, "&") + .replace(/"/g, """) + .replace(/</g, "<") + .replace(/>/g, ">"); + + markdownIt.renderer.rules.mention_open = ( tokens: MarkdownItToken[], idx: number, ) => { const token = tokens[idx]; - const id = - token.attrs.find((attr) => attr[0] === "id")?.[1] || ""; - const label = - token.attrs.find((attr) => attr[0] === "label")?.[1] || ""; - return `<span class="mention-label" data-type="mention" data-id="${id}" data-label="${label}" data-mention-suggestion-char="@" contenteditable="false">`; + const rawId = + token.attrs.find((attr) => attr[0] === "id")?.[1] || ""; + const rawLabel = + token.attrs.find((attr) => attr[0] === "label")?.[1] || ""; + const id = escapeAttr(rawId); + const label = escapeAttr(rawLabel); + return `<span class="mention-label" data-type="mention" data-id="${id}" data-label="${label}" data-mention-suggestion-char="@" contenteditable="false">`; };apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
32-35: Fix auth header handling and status code
- headers() is sync; remove unnecessary await.
- Return 401 for invalid API key to match auth failure semantics.
- if (!isValidInternalApiKey(await headers(), logger)) { + if (!isValidInternalApiKey(headers(), logger)) { logger.error("Invalid API key for sender pattern analysis", json); - return NextResponse.json({ error: "Invalid API key" }); + return NextResponse.json({ error: "Invalid API key" }, { status: 401 }); }
66-74: Reorder checks: 404 when account missing, then provider guardCurrent guard returns 400 for missing accounts due to optional chaining. Ensure not-found takes precedence, then enforce provider=google.
- if (emailAccount?.account?.provider !== "google") { - logger.warn("Unsupported provider", { emailAccountId }); - return NextResponse.json({ success: false }, { status: 400 }); - } - - if (!emailAccount) { + if (!emailAccount) { logger.error("Email account not found", { emailAccountId }); return NextResponse.json({ success: false }, { status: 404 }); } + + if (emailAccount.account?.provider !== "google") { + logger.warn("Unsupported provider", { + emailAccountId, + provider: emailAccount.account?.provider, + }); + return NextResponse.json({ success: false }, { status: 400 }); + }apps/web/utils/outlook/label.ts (2)
338-360: Fallback path: compute destinationFolderId before moving messagesdestinationFolderId is scoped to the primary try and isn’t available here. Compute it again in the fallback path before issuing move requests.
Add this line right after entering the inner try in the fallback path:
const destinationFolderId = await getOrCreateFolderByName(client, folderName);This ensures both per-message and single-message move calls use a valid folder ID.
328-335: Fallback path: conversationId not selected but usedYou call .select("id") and then filter by message.conversationId, which will always be undefined.
Apply this diff to include conversationId:
- .select("id") + .select("id,conversationId")apps/web/utils/reply-tracker/draft-tracking.ts (2)
98-106: Guard against undefined content before similarity calculationcalculateSimilarity likely expects strings. executedAction.content or sentTextPlain can be undefined/null, leading to NaN or runtime errors. Add null-safe guards or defaults.
- const similarityScore = calculateSimilarity( - executedAction.content, - sentTextPlain, - ); + const original = executedAction.content ?? ""; + const sent = sentTextPlain ?? ""; + const similarityScore = original && sent ? calculateSimilarity(original, sent) : 0;
41-61: Wrap core flow in try/catch to avoid unhandled errors and log contextgetDraft, Prisma calls, and the transaction can throw. Add a try/catch to prevent breaking the caller and to log failures with context (aligns with apps/web error-handling guideline).
export async function trackSentDraftStatus({ @@ }) { - const { threadId, id: sentMessageId, textPlain: sentTextPlain } = message; + const { threadId, id: sentMessageId, textPlain: sentTextPlain } = message; const loggerOptions = { threadId, sentMessageId }; - logger.info( + logger.info( "Checking if sent message corresponds to an AI draft", loggerOptions, ); - if (!sentMessageId) { + try { + if (!sentMessageId) { logger.warn("Sent message missing ID, cannot track draft status", { threadId, }); return; - } + } @@ - const executedAction = await prisma.executedAction.findFirst({ + const executedAction = await prisma.executedAction.findFirst({ @@ - const draftExists = await getDraft(executedAction.draftId, gmail); + const draftExists = await getDraft(executedAction.draftId, gmail); @@ - await prisma.$transaction([ + await prisma.$transaction([ prisma.draftSendLog.create({ data: { executedActionId: executedActionId, sentMessageId: sentMessageId, similarityScore: similarityScore, }, }), // Mark that the draft was sent prisma.executedAction.update({ where: { id: executedActionId }, data: { wasDraftSent: true }, }), ]); @@ - logger.info( + logger.info( "Successfully created draft send log and updated action status via transaction", loggerOptionsWithAction, ); + } catch (error) { + logger.error("trackSentDraftStatus failed", { + ...loggerOptions, + error: formatError(error), + }); + }(Consider mirroring the same try/catch structure in trackSentDraftStatusWithProvider.)
Also applies to: 86-94, 108-121
apps/web/utils/gmail/message.ts (1)
213-227: Bug: PUBLIC_DOMAINS contains values with '@', causing non-matchesextractDomainFromEmail returns a domain without '@'. Entries "@me.com" and "@hey.com" will never match, causing domain logic to misclassify.
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", ]);Also applies to: 233-241
apps/web/__tests__/ai/choose-rule/draft-management.test.ts (1)
9-16: Use the shared Prisma mock instead of a custom inline mockProject convention: vi.mock("@/utils/prisma") and use "@/utils/mocks/prisma" for consistency and maintainability. The custom factory overrides the provided mock.
-vi.mock("@/utils/prisma", () => ({ - default: { - executedAction: { - findFirst: vi.fn(), - update: vi.fn(), - }, - }, -})); +vi.mock("@/utils/prisma"); +import prismaMock from "@/utils/__mocks__/prisma"; @@ - const mockFindFirst = prisma.executedAction.findFirst as Mock; - const mockUpdate = prisma.executedAction.update as Mock; + const mockFindFirst = prismaMock.executedAction.findFirst as Mock; + const mockUpdate = prismaMock.executedAction.update as Mock;Also applies to: 32-37
apps/web/utils/cold-email/is-cold-email.ts (1)
126-134: Handle provider errors when checking previous communicationsprovider.hasPreviousCommunicationsWithSenderOrDomain may throw (network/transient). Guard with try/catch to avoid failing the cold-email determination flow and to log appropriately.
- 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("Provider check failed; proceeding to AI", { error }); + hasPreviousEmail = false; + } + }apps/web/app/api/user/stats/newsletters/route.ts (1)
139-156: Replace Raw Queries with Parameterized Prisma.sql in Newsletter Stats
The endpoint at apps/web/app/api/user/stats/newsletters/route.ts currently builds SQL via string concatenation and callsprisma.$queryRawUnsafe, which is hard to audit and risks injection. Please refactor:• Lines 139–156 (and similarly 158–175, 177–188, 189–205, 207–213):
– ReplacewhereConditions: string[]+queryParamswith an array ofPrisma.Sqlfragments (whereSql) and compose them viaPrisma.join.
– Swap outprisma.$queryRawUnsafe(query.sql, …params)forprisma.$queryRaw(query)using thePrisma.Sqlobject you’ve built.
• EnsuregetOrderByClauseemits only fixed, whitelisted SQL (or refactor it into aPrisma.sql-based switch if it ever becomes dynamic).Example diff (lines 139–156):
- const whereConditions: string[] = []; - const queryParams: Array<string | Date> = []; + const whereSql: Prisma.Sql[] = []; - if (options.fromDate) { - whereConditions.push( - `"date" >= to_timestamp($${queryParams.length + 1}::double precision)`, - ); - queryParams.push((options.fromDate / 1000).toString()); - } + if (options.fromDate) { + whereSql.push( + Prisma.sql`"date" >= to_timestamp(${options.fromDate / 1000}::double precision)`, + ); + } … - const results = await prisma.$queryRawUnsafe<NewsletterCountRawResult[]>( - query.sql, - ...queryParams, - ...query.values, - ); + const results = await prisma.$queryRaw<NewsletterCountRawResult[]>(query);Applying this pattern consistently across all newsletter‐stats queries will standardize parameter binding, improve readability, and eliminate injection risks.
apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (1)
206-215: Remove console.error in client codeGuidelines disallow console usage in app code. You already show a toast; drop the console.error.
- } catch (error) { - console.error(`Failed to process message ${message.id}:`, error); + } catch (error) { toastError({ title: "Failed to process email", description: `Error processing email from ${message.headers.from}: ${error instanceof Error ? error.message : "Unknown error"}`, });apps/web/utils/email/provider.ts (1)
9-24: Improve error handling and input validationThe function should provide more informative error messages and validate inputs properly.
+const SUPPORTED_PROVIDERS = ["google", "microsoft"] as const; +type SupportedProvider = typeof SUPPORTED_PROVIDERS[number]; + export async function createEmailProvider({ emailAccountId, provider, }: { emailAccountId: string; - provider: string | null; + provider: string | null; }): Promise<EmailProvider> { + if (!emailAccountId?.trim()) { + throw new Error("emailAccountId is required and cannot be empty"); + } + if (provider === "google") { const client = await getGmailClientForEmail({ emailAccountId }); return new GmailProvider(client); } else if (provider === "microsoft") { const client = await getOutlookClientForEmail({ emailAccountId }); return new OutlookProvider(client); } - throw new Error(`Unsupported provider: ${provider}`); + throw new Error( + `Unsupported provider: ${provider}. Supported providers are: ${SUPPORTED_PROVIDERS.join(", ")}` + ); }
🧹 Nitpick comments (25)
.gitignore (1)
76-78: Pattern ‘logs’ matches at any depth; use ‘/logs/’ if you only want the root folder ignoredCurrently, “logs” will ignore any directory/file named logs anywhere in the repo, not just at the top level.
- If intention is root-only CLI logs: change to “/logs/”.
- If you want to ignore log files generally, consider adding “.log” (and maybe “.log.*”).
- If you plan to keep a placeholder committed, add “!logs/.gitkeep”.
Suggested diff:
-# cli logs -logs +# cli logs (root only) +/logs/ +# optionally, also ignore generic log files across the repo +# *.log +# *.log.* +# keep placeholder if needed +# !logs/.gitkeepapps/web/utils/llms/model.ts (2)
42-53: Add explicit return type to selectModelByTypeLLM-related functions should declare parameter and return types. Add : SelectModel to the function signature.
-function selectModelByType(userAi: UserAIFields, modelType: ModelType) { +function selectModelByType( + userAi: UserAIFields, + modelType: ModelType, +): SelectModel {
367-377: Annotate return type of getProviderApiKeyExplicit return types are required for LLM-related functions.
-function getProviderApiKey(provider: string) { +function getProviderApiKey(provider: string): string | undefined { const providerApiKeys: Record<string, string | undefined> = { [Provider.ANTHROPIC]: env.ANTHROPIC_API_KEY, [Provider.OPEN_AI]: env.OPENAI_API_KEY, [Provider.GOOGLE]: env.GOOGLE_API_KEY, [Provider.GROQ]: env.GROQ_API_KEY, [Provider.OPENROUTER]: env.OPENROUTER_API_KEY, }; return providerApiKeys[provider]; }apps/web/app/api/ai/analyze-sender-pattern/route.ts (2)
23-27: Tighten input validation (Zod refine)Consider validating emailAccountId format (e.g., cuid/uuid) and ensuring extractEmailAddress(from) is non-empty.
-const schema = z.object({ - emailAccountId: z.string(), - from: z.string(), -}); +const schema = z.object({ + emailAccountId: z.string().min(1, "emailAccountId required"), + from: z.string().min(1, "from required"), +}); @@ -const from = extractEmailAddress(data.from); +const from = extractEmailAddress(data.from); +if (!from) { + return NextResponse.json({ error: "Invalid from address" }, { status: 400 }); +}Also applies to: 37-41
230-252: Optional: parallelize per-thread message fetchesFetching thread messages sequentially increases latency. For small MAX_RESULTS it’s fine, but you can speed up with Promise.all and short-circuit on conversation detection.
- for (const thread of threads.threads) { - const messages = await getThreadMessages(thread.id, gmail); + const all = await Promise.all( + threads.threads.map(async (thread) => ({ + threadId: thread.id, + messages: await getThreadMessages(thread.id, gmail), + })), + ); - // Check if this is a conversation (multiple senders) - const senders = messages.map((msg) => + for (const { threadId, messages } of all) { + const senders = messages.map((msg) => extractEmailAddress(msg.headers.from), ); @@ - threadsWithMessages.push({ - threadId: thread.id, - messages, - }); + threadsWithMessages.push({ threadId, messages });apps/web/__tests__/helpers.ts (1)
4-7: Support nested overrides when building EmailAccountWithAIOnly email is overridable now; allow overriding nested user fields to make tests more flexible.
export function getEmailAccount( overrides: Partial<EmailAccountWithAI> = {}, ): EmailAccountWithAI { - return { + const base: EmailAccountWithAI = { id: "email-account-id", userId: "user1", - email: overrides.email || "user@test.com", + email: "user@test.com", about: null, user: { aiModel: null, aiProvider: null, aiApiKey: null, }, }; + return { + ...base, + ...overrides, + email: overrides.email ?? base.email, + user: { + ...base.user, + ...(overrides.user ?? {}), + }, + }; }Also applies to: 10-17
apps/web/utils/outlook/label.ts (1)
301-305: Nit: Include both folderName and destinationFolderId in logs for better observabilityCurrent logs mention folderName; including the resolved destinationFolderId helps diagnose issues when names map to well-known folders or custom-created ones.
Consider adding destinationFolderId to log contexts where available.
Also applies to: 309-313, 379-385, 390-393
apps/web/utils/email/types.ts (3)
137-140: Use T[] consistently instead of ArrayThe guidelines prefer consistent array typing. This line uses Array<...> whereas most others use T[].
Apply this diff:
- ): Promise<Array<{ id: string; snippet: string; subject: string }>>; + ): Promise<{ id: string; snippet: string; subject: string }[]>;
1-4: Decouple utils types from app-layer validation (imports from app/)Importing ThreadsQuery from "@/app/api/threads/validation" couples utils to the app layer. It risks circular deps and complicates reuse.
Extract threadsQuery and ThreadsQuery to a shared module under utils (e.g., apps/web/utils/validation/threads.ts). Re-export from app routes as needed so both layers depend on utils, not vice versa. I can draft this refactor if helpful.
Also applies to: 36-165
36-165: Optional: preferexport typeoverinterfacefor consistency with guidelinesNot urgent, but to align with “Use export type for types,” consider switching interfaces to exported type aliases.
Example:
export type EmailLabel = { id: string; name: string; // ... };apps/web/utils/ai/choose-rule/match-rules.test.ts (1)
539-545: Nit: Fix test name typoMinor spelling fix for readability.
- it("doens't match wrong static domain", async () => { + it("doesn't match wrong static domain", async () => {apps/web/utils/ai/choose-rule/execute.ts (1)
75-80: Log action execution errors before rethrowingWe flip the executed rule to ERROR and rethrow. Add structured logging for triage.
} catch (error) { await prisma.executedRule.update({ where: { id: executedRule.id }, data: { status: ExecutedRuleStatus.ERROR }, }); - throw error; + logger.error("Action execution failed", { + actionId: action.id, + actionType: action.type, + error, + }); + throw error; }apps/web/utils/ai/choose-rule/draft-management.ts (1)
55-61: Use similarity threshold instead of exact string equalityExact equality after trimming and simple quote-header stripping is brittle. Using calculateSimilarity with a high threshold (e.g., >= 0.99) reduces false negatives for minor whitespace/punctuation edits and aligns with how other modules assess draft changes.
+import { calculateSimilarity } from "@/utils/similarity-score"; @@ - if (originalContent === currentReplyContent) { + const similarity = calculateSimilarity( + originalContent ?? "", + currentReplyContent ?? "", + ); + if (similarity >= 0.99) { logger.info("Draft content matches, deleting draft."); @@ - } else { - logger.info("Draft content modified by user, skipping deletion."); - } + } else { + logger.info("Draft content modified by user, skipping deletion.", { + similarity, + }); + }Also applies to: 72-80
apps/web/app/(app)/[emailAccountId]/assistant/ActionSummaryCard.tsx (1)
193-196: Refine MOVE_FOLDER summary for consistency and whitespace handlingMinor UX nit: trim folder name and keep display consistent with other actions.
- case ActionType.MOVE_FOLDER: - summaryContent = `Folder: ${action.folderName?.value || "unset"}`; - break; + case ActionType.MOVE_FOLDER: { + const folder = action.folderName?.value?.trim(); + summaryContent = `Folder: ${folder || "unset"}`; + break; + }apps/web/utils/ai/rule/create-prompt-from-rule.ts (1)
95-98: Include folder name in prompt when availableImproves prompt specificity and downstream LLM behavior without changing control flow.
- case ActionType.MOVE_FOLDER: - actions.push("move to folder"); - break; + case ActionType.MOVE_FOLDER: + if (action.folderName) { + actions.push(`move to folder "${action.folderName}"`); + } else { + actions.push("move to folder"); + } + break;apps/web/utils/action-display.ts (1)
26-29: Nit: trim folderName to avoid odd spacingMinor polish to handle accidental leading/trailing spaces.
- case ActionType.MOVE_FOLDER: - return action.folderName - ? `Folder: ${action.folderName}` - : "Move to folder"; + case ActionType.MOVE_FOLDER: { + const name = action.folderName?.trim(); + return name ? `Folder: ${name}` : "Move to folder"; + }apps/web/utils/ai/rule/generate-rules-prompt.ts (3)
48-50: System prompt is clear; consider tightening output constraintsYou already enforce a schema at generation time; still, consider explicitly stating “Return only JSON matching the provided schema. Do not include prose.” to reduce LLM drift.
129-144: Trim trailing newlines from formatted rulesparseRulesResponse adds a trailing newline to each formatted item. Consider returning items without trailing newlines to simplify downstream rendering.
- return parsedRules.rules.map(({ rule, snippet }) => { - const formattedRule = `* ${rule}\n`; - if (snippet) return `${formattedRule}${formatSnippet(snippet)}\n`; - return formattedRule; - }); + return parsedRules.rules.map(({ rule, snippet }) => { + const formattedRule = `* ${rule}`; + if (snippet) return `${formattedRule}\n${formatSnippet(snippet)}`; + return formattedRule; + });
115-120: Prompt hygiene and logging (optional)
- Consider trimming and truncating long inputs (about, snippets, emails) before interpolation to keep tokens bounded. You already limit lastSentEmails; you can also trim snippet/email text.
- createGenerateObject logs inputs/outputs; if needed, add a scoped logger here to log hasSnippets, counts, and lengths rather than content.
Also applies to: 146-151
apps/web/__tests__/ai/reply/reply-context-collector.test.ts (2)
11-13: Use standard AI test timeoutsGuideline suggests 15,000ms for long-running AI tests. If feasible, reduce TEST_TIMEOUT to 15_000; otherwise document why 60_000ms is required.
89-106: Use console.debug for AI test logsLLM-related tests should prefer console.debug over console.log. Swap these calls accordingly.
- console.log( + console.debug( `Basic: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Technical support: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Escalation: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Billing: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Shipping: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Product inquiry: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `Security: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); - console.log("result", result); + console.debug("result", result); @@ - console.log( + console.debug( `No-history: LLM issued ${observedQueries.length} search call(s):`, observedQueries, ); @@ - console.log("Subject line search queries:", observedQueries); + console.debug("Subject line search queries:", observedQueries);Also applies to: 150-167, 241-258, 315-332, 389-411, 467-485, 541-558, 615-632, 688-691, 769-769
apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (1)
104-115: Encode the query using URLSearchParamsJoining IDs with commas risks edge cases and URL length issues. Prefer URLSearchParams with repeated keys.
-const { data: existingRules } = useSWR<BatchExecutedRulesResponse>( - messageIdsToFetch.length > 0 - ? `/api/user/executed-rules/batch?messageIds=${messageIdsToFetch.join(",")}` - : null, -); +const { data: existingRules } = useSWR<BatchExecutedRulesResponse>(() => { + if (messageIdsToFetch.length === 0) return null; + const qs = new URLSearchParams(); + for (const id of messageIdsToFetch) qs.append("messageIds", id); + return `/api/user/executed-rules/batch?${qs.toString()}`; +});apps/web/utils/actions/rule.validation.ts (1)
73-90: Folder path validation: add simple length boundsValidation is solid (no empty segments or //). Consider adding length bounds to prevent extreme inputs.
- value: z - .string() + value: z + .string() + .max(256, "Folder path is too long") .nullish() .refine((val) => { if (!val?.trim()) return true; // Check for empty folder parts if (val.includes("//") || val.split("/").some((part) => !part.trim())) return false; return true; }),apps/web/utils/ai/rule/create-rule.ts (1)
6-9: Consider adding explicit return type annotationThe function lost its explicit return type annotation. Consider adding it back for better type safety and documentation.
export async function aiCreateRule( instructions: string, emailAccount: EmailAccountWithAI, -) { +): Promise<z.infer<typeof createRuleSchema>> {apps/web/utils/email/microsoft.ts (1)
689-693: Simplify nextPageToken extraction logicThe nested ternary operator makes the code harder to read. Consider extracting this logic to improve readability.
+ const extractSkipToken = (nextLink: string | undefined): string | undefined => { + if (!nextLink) return undefined; + try { + return new URL(nextLink).searchParams.get("$skiptoken") || undefined; + } catch { + return undefined; + } + }; + return { threads, - nextPageToken: response["@odata.nextLink"] - ? new URL(response["@odata.nextLink"]).searchParams.get("$skiptoken") || - undefined - : undefined, + nextPageToken: extractSkipToken(response["@odata.nextLink"]), };
| import { withEmailAccount } from "@/utils/middleware"; | ||
| import prisma from "@/utils/prisma"; | ||
|
|
||
| const batchRequestSchema = z.object({ messageIds: z.array(z.string()) }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain and sanitize messageIds
Add bounds and sanitization to avoid excessive IN queries and empty IDs.
-const batchRequestSchema = z.object({ messageIds: z.array(z.string()) });
+const batchRequestSchema = z.object({
+ messageIds: z
+ .array(z.string().min(1))
+ .min(1, "Provide at least one messageId")
+ .max(100, "You can query up to 100 IDs at a time")
+ .transform((ids) => Array.from(new Set(ids.filter(Boolean)))),
+});📝 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 batchRequestSchema = z.object({ messageIds: z.array(z.string()) }); | |
| -const batchRequestSchema = z.object({ messageIds: z.array(z.string()) }); | |
| +const batchRequestSchema = z.object({ | |
| + messageIds: z | |
| + .array(z.string().min(1)) | |
| + .min(1, "Provide at least one messageId") | |
| + .max(100, "You can query up to 100 IDs at a time") | |
| + .transform((ids) => Array.from(new Set(ids.filter(Boolean)))), | |
| +}); |
🤖 Prompt for AI Agents
In apps/web/app/api/user/executed-rules/batch/route.ts around line 6, the
batchRequestSchema currently allows arbitrary arrays of strings; constrain and
sanitize messageIds by requiring a non-empty array with sensible bounds (e.g.
min 1, max 100), enforce each id is a trimmed, non-empty string with a
reasonable length limit (e.g. max 200), and ensure ids are unique; implement
these rules using zod validators/transforms (trim each string, validate length
and non-empty, then refine the array length and uniqueness) so the server
rejects empty/oversized/duplicate IDs and avoids excessive IN queries.
| <DropdownMenuItem | ||
| key={emailAccount.id} | ||
| className="gap-2 p-2" | ||
| onSelect={() => { | ||
| // Force a hard page reload to refresh all data. | ||
| // I tried to fix with resetting the SWR cache but it didn't seem to work. This is much more reliable anyway. | ||
| window.location.href = getHref(emailAccount.id); | ||
| }} | ||
| > | ||
| <ProfileImage | ||
| image={emailAccount.image} | ||
| label={emailAccount.name || emailAccount.email} | ||
| /> | ||
| <div className="flex flex-col"> | ||
| <span className="truncate font-medium"> | ||
| {emailAccount.name || emailAccount.email} | ||
| </span> | ||
| {emailAccount.name && ( | ||
| <span className="truncate text-xs text-muted-foreground"> | ||
| {emailAccount.email} | ||
| </span> | ||
| {emailAccount.name && ( | ||
| <span className="truncate text-xs text-muted-foreground"> | ||
| {emailAccount.email} | ||
| </span> | ||
| )} | ||
| </div> | ||
| </DropdownMenuItem> | ||
| </Link> | ||
| )} | ||
| </div> | ||
| </DropdownMenuItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid window.location; keep native link semantics with full reload via Link + asChild
Using window.location drops a11y (no open-in-new-tab, lost context menu) and bypasses Next’s link semantics. You can still force a hard reload with Link’s reloadDocument while preserving UX/accessibility.
Apply this diff:
- {emailAccounts.map((emailAccount) => (
- <DropdownMenuItem
- key={emailAccount.id}
- className="gap-2 p-2"
- onSelect={() => {
- // Force a hard page reload to refresh all data.
- // I tried to fix with resetting the SWR cache but it didn't seem to work. This is much more reliable anyway.
- window.location.href = getHref(emailAccount.id);
- }}
- >
- <ProfileImage
- image={emailAccount.image}
- label={emailAccount.name || emailAccount.email}
- />
- <div className="flex flex-col">
- <span className="truncate font-medium">
- {emailAccount.name || emailAccount.email}
- </span>
- {emailAccount.name && (
- <span className="truncate text-xs text-muted-foreground">
- {emailAccount.email}
- </span>
- )}
- </div>
- </DropdownMenuItem>
- ))}
+ {emailAccounts.map((emailAccount) => (
+ <DropdownMenuItem key={emailAccount.id} asChild className="p-0">
+ <Link
+ href={getHref(emailAccount.id)}
+ prefetch={false}
+ reloadDocument
+ className="flex w-full items-center gap-2 p-2"
+ >
+ <ProfileImage
+ image={emailAccount.image}
+ label={emailAccount.name || emailAccount.email}
+ />
+ <div className="flex flex-col">
+ <span className="truncate font-medium">
+ {emailAccount.name || emailAccount.email}
+ </span>
+ {emailAccount.name && (
+ <span className="truncate text-xs text-muted-foreground">
+ {emailAccount.email}
+ </span>
+ )}
+ </div>
+ </Link>
+ </DropdownMenuItem>
+ ))}📝 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.
| <DropdownMenuItem | |
| key={emailAccount.id} | |
| className="gap-2 p-2" | |
| onSelect={() => { | |
| // Force a hard page reload to refresh all data. | |
| // I tried to fix with resetting the SWR cache but it didn't seem to work. This is much more reliable anyway. | |
| window.location.href = getHref(emailAccount.id); | |
| }} | |
| > | |
| <ProfileImage | |
| image={emailAccount.image} | |
| label={emailAccount.name || emailAccount.email} | |
| /> | |
| <div className="flex flex-col"> | |
| <span className="truncate font-medium"> | |
| {emailAccount.name || emailAccount.email} | |
| </span> | |
| {emailAccount.name && ( | |
| <span className="truncate text-xs text-muted-foreground"> | |
| {emailAccount.email} | |
| </span> | |
| {emailAccount.name && ( | |
| <span className="truncate text-xs text-muted-foreground"> | |
| {emailAccount.email} | |
| </span> | |
| )} | |
| </div> | |
| </DropdownMenuItem> | |
| </Link> | |
| )} | |
| </div> | |
| </DropdownMenuItem> | |
| {emailAccounts.map((emailAccount) => ( | |
| <DropdownMenuItem key={emailAccount.id} asChild className="p-0"> | |
| <Link | |
| href={getHref(emailAccount.id)} | |
| prefetch={false} | |
| reloadDocument | |
| className="flex w-full items-center gap-2 p-2" | |
| > | |
| <ProfileImage | |
| image={emailAccount.image} | |
| label={emailAccount.name || emailAccount.email} | |
| /> | |
| <div className="flex flex-col"> | |
| <span className="truncate font-medium"> | |
| {emailAccount.name || emailAccount.email} | |
| </span> | |
| {emailAccount.name && ( | |
| <span className="truncate text-xs text-muted-foreground"> | |
| {emailAccount.email} | |
| </span> | |
| )} | |
| </div> | |
| </Link> | |
| </DropdownMenuItem> | |
| ))} |
🤖 Prompt for AI Agents
In apps/web/components/AccountSwitcher.tsx around lines 116 to 139, replace the
window.location.href hard-reload inside the DropdownMenuItem onSelect with
Next.js Link using asChild and reloadDocument so native link semantics and
accessibility (open-in-new-tab, context menu) are preserved; wrap the
DropdownMenuItem (or make it the child) with Link
href={getHref(emailAccount.id)} reloadDocument and remove the onSelect
window.location call, keeping the ProfileImage and inner markup unchanged.
| onSelect={() => { | ||
| // Force a hard page reload to refresh all data. | ||
| // I tried to fix with resetting the SWR cache but it didn't seem to work. This is much more reliable anyway. | ||
| window.location.href = getHref(emailAccount.id); | ||
| }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
getHref can mis-replace path segments; make it segment-aware and encode ids
basePath.replace(activeEmailAccountId, emailAccountId) is fragile (e.g., id appears multiple times or not at all). Also consider URL-encoding. Suggest a segment-based approach.
Proposed replacement for getHref (outside this hunk):
const getHref = useCallback(
(emailAccountId: string) => {
const tab = searchParams.get("tab") || "";
const basePath = pathname.split("?")[0] || "/";
const segments = basePath.split("/").filter(Boolean);
const nextId = encodeURIComponent(emailAccountId);
// If we don't have an active id or the first segment isn't the active id,
// default to the setup route for the target account.
if (!activeEmailAccountId || segments[0] !== activeEmailAccountId) {
return `/${nextId}/setup${tab ? `?tab=${encodeURIComponent(tab)}` : ""}`;
}
// Replace only the first path segment (account id).
segments[0] = nextId;
const newBasePath = `/${segments.join("/")}`;
return `${newBasePath}${tab ? `?tab=${encodeURIComponent(tab)}` : ""}`;
},
[pathname, activeEmailAccountId, searchParams],
);Please verify behavior when:
- Current route doesn’t contain the active id (e.g., /accounts).
- Path contains the id substring in other segments.
- tab contains characters needing encoding.
🏁 Script executed:
#!/bin/bash
# Search for the getHref definition and any use of basePath.replace in AccountSwitcher.tsx
rg --context=5 "const getHref" -n apps/web/components/AccountSwitcher.tsx
rg --context=5 "basePath.replace" -n apps/web/components/AccountSwitcher.tsxLength of output: 887
Make getHref segment-aware and URL-encode IDs
The current implementation at apps/web/components/AccountSwitcher.tsx (lines 50–60) uses
const newBasePath = basePath.replace(
activeEmailAccountId,
emailAccountId,
);This approach is fragile because:
- If the active ID doesn’t appear in the path, replace does nothing (you’ll stay on the wrong route).
- If the ID appears in multiple segments, all occurrences get replaced.
- Neither the new account ID nor the tab query value is URL-encoded.
Suggested refactor for getHref:
const getHref = useCallback(
(emailAccountId: string) => {
const tab = searchParams.get("tab") || "";
// Strip off any query string
const basePath = pathname.split("?")[0] || "/";
// Split into path segments, dropping empty entries
const segments = basePath.split("/").filter(Boolean);
const nextId = encodeURIComponent(emailAccountId);
const encodedTab = tab ? `?tab=${encodeURIComponent(tab)}` : "";
// If no active ID in the first segment, go to setup for the target
if (!activeEmailAccountId || segments[0] !== activeEmailAccountId) {
return `/${nextId}/setup${encodedTab}`;
}
// Replace only the first segment (account ID) and reassemble
segments[0] = nextId;
const newBasePath = `/${segments.join("/")}`;
return `${newBasePath}${encodedTab}`;
},
[pathname, activeEmailAccountId, searchParams],
);• apps/web/components/AccountSwitcher.tsx: adjust lines 50–60
• Ensure encodeURIComponent is used for both emailAccountId and tab
• Handle missing or duplicate IDs by operating on path segments only
🤖 Prompt for AI Agents
In apps/web/components/AccountSwitcher.tsx around lines 119-123, update getHref
to be segment-aware and URL-encode IDs and tab values: strip any query from
pathname, split the path into segments (dropping empty entries), encode the
target emailAccountId and tab with encodeURIComponent, if activeEmailAccountId
is missing or not the first segment return "/{encodedId}/setup{encodedTab}",
otherwise replace only the first segment with the encodedId, reassemble the path
and append the encoded tab query if present; ensure onSelect uses this robust
getHref so IDs aren’t blindly replaced across the whole path and query values
are encoded.
| await createContact(email, name?.split(" ")[0]).catch((error) => { | ||
| // ignore if already exists | ||
| if (error.message.includes("Email already on list")) { | ||
| logger.info("Email already on list", { email }); | ||
| return; | ||
| } | ||
|
|
||
| throw error; | ||
| }); |
There was a problem hiding this comment.
Robustly narrow unknown errors before using error.message.includes
As written, error.message.includes can throw if error is not an Error or message is missing. Guard the shape before calling includes.
- await createContact(email, name?.split(" ")[0]).catch((error) => {
- // ignore if already exists
- if (error.message.includes("Email already on list")) {
- logger.info("Email already on list", { email });
- return;
- }
-
- throw error;
- });
+ await createContact(email, name?.split(" ")[0]).catch((err: unknown) => {
+ // ignore if already exists
+ const message =
+ err && typeof err === "object" && "message" in err && typeof (err as any).message === "string"
+ ? (err as any).message
+ : "";
+ if (message.includes("Email already on list")) {
+ logger.info("Email already on list", { email });
+ return;
+ }
+ throw err;
+ });🤖 Prompt for AI Agents
In apps/web/ee/billing/stripe/loops-events.ts around lines 47 to 55, the catch
block calls error.message.includes which can throw if error is not an Error or
message is missing; update the guard to check the error shape first (e.g., if
(error instanceof Error && typeof error.message === "string" &&
error.message.includes("Email already on list")) { ... }) or coerce safely
(e.g., const msg = String((error && (error as any).message) ?? error); if
(msg.includes(...)) ...) so you only call includes on a guaranteed string, and
retain the existing behavior of logging and rethrowing for other errors.
| }): 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
Avoid any in public interfaces; return precise types
Using Promise weakens type-safety across the codebase. Define concrete return types for filter operations.
Suggested changes:
- createFilter: Promise
- deleteFilter: Promise
- createAutoArchiveFilter: Promise
- }): Promise<any>;
+ }): Promise<EmailFilter>;
- deleteFilter(id: string): Promise<any>;
+ deleteFilter(id: string): Promise<void>;
- }): Promise<any>;
+ }): Promise<EmailFilter>;📝 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.
| }): Promise<any>; | |
| deleteFilter(id: string): Promise<any>; | |
| createAutoArchiveFilter(options: { | |
| from: string; | |
| gmailLabelId?: string; | |
| labelName?: string; | |
| }): Promise<any>; | |
| }): Promise<EmailFilter>; | |
| deleteFilter(id: string): Promise<void>; | |
| createAutoArchiveFilter(options: { | |
| from: string; | |
| gmailLabelId?: string; | |
| labelName?: string; | |
| }): Promise<EmailFilter>; |
🤖 Prompt for AI Agents
In apps/web/utils/email/types.ts around lines 95 to 101, the interface currently
uses Promise<any> for filter operations which weakens type-safety; change the
method signatures to use concrete types: createFilter(...):
Promise<EmailFilter>, deleteFilter(id: string): Promise<void>,
createAutoArchiveFilter(...): Promise<EmailFilter>; ensure an EmailFilter
type/interface is defined or imported in this file and exported if needed, and
update implementation files and callers to return/resolve the correct
EmailFilter objects (or void for delete) so the type signatures match runtime
behavior.
| if (userAi.aiApiKey) return selectDefaultModel(userAi); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Early return now ignores "economy" and "chat" model types when a user API key is present — likely unintended regression
This guard causes selectModelByType to always route to selectDefaultModel for any modelType whenever userAi.aiApiKey is truthy, bypassing selectEconomyModel/selectChatModel. If callers pass modelType = "economy" or "chat", those branches will never execute for users with an API key.
If the intent is to continue honoring modelType even when a user has their own key, remove the guard. If the intent is only to shortcut the "default" path, scope the guard to modelType === "default" and keep routing economy/chat as before.
Option A — revert the guard and add an explicit return type:
-function selectModelByType(userAi: UserAIFields, modelType: ModelType) {
- if (userAi.aiApiKey) return selectDefaultModel(userAi);
+function selectModelByType(
+ userAi: UserAIFields,
+ modelType: ModelType,
+): SelectModel {
switch (modelType) {
case "economy":
return selectEconomyModel(userAi);
case "chat":
return selectChatModel(userAi);
default:
return selectDefaultModel(userAi);
}
}Option B — only shortcut the "default" path (still add return type):
-function selectModelByType(userAi: UserAIFields, modelType: ModelType) {
- if (userAi.aiApiKey) return selectDefaultModel(userAi);
+function selectModelByType(
+ userAi: UserAIFields,
+ modelType: ModelType,
+): SelectModel {
switch (modelType) {
case "economy":
return selectEconomyModel(userAi);
case "chat":
return selectChatModel(userAi);
default:
+ if (userAi.aiApiKey) return selectDefaultModel(userAi);
return selectDefaultModel(userAi);
}
}To quantify impact of current behavior, run:
🏁 Script executed:
#!/bin/bash
# Find all call sites that request 'economy' or 'chat' models.
rg -n -H -S $'getModel\\([^,]+,\\s*"(economy|chat)"' -A2 -B2
# Also check any direct uses of selectModelByType (if any).
rg -n -H 'selectModelByType\(' -A2 -B2Length of output: 8542
🏁 Script executed:
#!/bin/bash
# Display the selectModelByType function and its surrounding context
sed -n '1,80p' apps/web/utils/llms/model.ts
echo "----"
sed -n '80,160p' apps/web/utils/llms/model.tsLength of output: 5213
Adjust early‐return guard in selectModelByType to only shortcut “default”
The unconditional if (userAi.aiApiKey) at lines 43–44 causes every call—regardless of modelType—to fall back to the default model whenever a user API key is set, effectively ignoring "economy" and "chat". Scope this guard to the default branch (or remove it) and add an explicit return type.
• File: apps/web/utils/llms/model.ts
• Location: selectModelByType (lines 42–44)
Suggested change (Option B – only shortcut default, plus return type):
-function selectModelByType(userAi: UserAIFields, modelType: ModelType) {
+function selectModelByType(
+ userAi: UserAIFields,
+ modelType: ModelType,
+): SelectModel {
switch (modelType) {
case "economy":
return selectEconomyModel(userAi);
case "chat":
return selectChatModel(userAi);
default:
- return selectDefaultModel(userAi);
+ if (userAi.aiApiKey) return selectDefaultModel(userAi);
+ return selectDefaultModel(userAi);
}
}• Add : SelectModel to the selectModelByType signature.
• Verify behavior for users with an API key when requesting "economy"/"chat".
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/utils/llms/model.ts around lines 42 to 44, the early-return guard
"if (userAi.aiApiKey) return selectDefaultModel(userAi);" incorrectly forces the
default model for all modelType values; change the function signature to include
the explicit return type ": SelectModel" and move or restrict the aiApiKey
shortcut so it only triggers when modelType === "default" (or remove it
entirely), ensuring requests for "economy" and "chat" still go through their
normal selection paths and verifying behavior for users with an API key.
| try { | ||
| const emailAccount = await getEmailAccountWithRules({ emailAccountId }); | ||
|
|
||
| if (emailAccount?.account?.provider !== "google") { |
There was a problem hiding this comment.
Analyze sender pattern is not implemented for Outlook
| .metadata({ name: "processPreviousSentEmails" }) | ||
| .action(async ({ ctx: { emailAccountId } }) => { | ||
| const emailAccount = await getEmailAccountWithAi({ emailAccountId }); | ||
| const emailAccount = await prisma.emailAccount.findUnique({ |
There was a problem hiding this comment.
Reply tracking is only implemented for Gmail, so I changed the query here to also fetch the provider and skip it if is not Gmail.
| ALTER TABLE "ScheduledAction" ADD COLUMN "folderName" TEXT; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "VerificationToken" ALTER COLUMN "id" DROP DEFAULT; |
There was a problem hiding this comment.
Fixing the default generated from the better-auth migration
| } | ||
|
|
||
| // Used by Outlook and Gmail providers | ||
| export async function saveTokens({ |
There was a problem hiding this comment.
Bringing back the original saveTokens implementation from main.
| async archiveThreadWithLabel( | ||
| threadId: string, | ||
| ownerEmail: string, | ||
| labelId?: string, |
There was a problem hiding this comment.
This param is never used in outlook.
| ownerEmail, | ||
| actionSource, | ||
| labelId, | ||
| folderName = "archive", |
There was a problem hiding this comment.
Modified the Outlook that archives to support other folder names (custom and well-known values).
If the folder is well-known, the id is the same as the folder name, e.g., the "archive" folder id is "archive", the inbox folder is "inbox", and so forth. But custom folders have to be referenced by id.
| return folderIdCache; | ||
| } | ||
|
|
||
| export async function getOrCreateFolderByName( |
There was a problem hiding this comment.
Create folders by name, or subfolders within folders.
Inbox/Newsletter
Inbox = well-known, so no creation
Newsletter = new or existing folder within the parent inbox
Custom/Some name
Custom = Custom folder (created or retrieved)
Some name = Custom sub-folder created or retrieved within the parent Custom
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/utils/actions/reply-tracking.ts (4)
45-47: Confirm desired UX for non-Google accounts (silent no-op vs surfaced “unsupported”)Other routes in this PR surface provider gating errors; here we return success and no-op for non-Google. If consistency is desired, consider throwing a SafeError to inform the caller; otherwise, document the intentional no-op.
54-55: Revalidate UI after processing to reflect new trackersThis action mutates data shown in Reply Zero; revalidate cache like in enableReplyTrackerAction.
- return { success: true }; + revalidatePath(prefixPath(emailAccountId, "/reply-zero")); + return { success: true };
33-43: Minimize selected fields to reduce PII exposure (aiApiKey etc.)If processPreviousSentEmails doesn’t use user.aiProvider/aiModel/aiApiKey here, drop them from the select to avoid handling sensitive data unnecessarily.
18-18: Nit: rename logger scope to match file domainThe scope “enableReplyTracker” is misleading in this module. Consider “reply-tracking”.
-const logger = createScopedLogger("enableReplyTracker"); +const logger = createScopedLogger("reply-tracking");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/(app)/[emailAccountId]/assistant/RuleForm.tsx(4 hunks)apps/web/prisma/migrations/20250811130806_add_move_folder_action/migration.sql(1 hunks)apps/web/prisma/schema.prisma(4 hunks)apps/web/utils/actions/reply-tracking.ts(1 hunks)apps/web/utils/outlook/label.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/prisma/schema.prisma
- apps/web/app/(app)/[emailAccountId]/assistant/RuleForm.tsx
🧰 Additional context used
📓 Path-based instructions (10)
!{.cursor/rules/*.mdc}
📄 CodeRabbit Inference Engine (.cursor/rules/cursor-rules.mdc)
Never place rule files in the project root, in subdirectories outside .cursor/rules, or in any other location
Files:
apps/web/prisma/migrations/20250811130806_add_move_folder_action/migration.sqlapps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
!pages/_document.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
!pages/_document.{js,jsx,ts,tsx}: Don't import next/document outside of pages/_document.jsx in Next.js projects.
Don't import next/document outside of pages/_document.jsx in Next.js projects.
Files:
apps/web/prisma/migrations/20250811130806_add_move_folder_action/migration.sqlapps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
apps/web/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (apps/web/CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TypeScript with strict null checks
Path aliases: Use@/for imports from project root
Use proper error handling with try/catch blocks
Format code with Prettier
Leverage TypeScript inference for better DX
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
apps/web/utils/actions/**/*.ts
📄 CodeRabbit Inference Engine (apps/web/CLAUDE.md)
apps/web/utils/actions/**/*.ts: Use server actions for all mutations (create/update/delete operations)
next-safe-actionprovides centralized error handling
Use Zod schemas for validation on both client and server
UserevalidatePathin server actions for cache invalidation
apps/web/utils/actions/**/*.ts: Use server actions (withnext-safe-action) for all mutations (create/update/delete operations); do NOT use POST API routes for mutations.
UserevalidatePathin server actions to invalidate cache after mutations.
Files:
apps/web/utils/actions/reply-tracking.ts
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/form-handling.mdc)
**/*.ts: The same validation should be done in the server action too
Define validation schemas using Zod
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/logging.mdc)
**/*.{ts,tsx}: UsecreateScopedLoggerfor logging in backend TypeScript files
Typically add the logger initialization at the top of the file when usingcreateScopedLogger
Only use.with()on a logger instance within a specific function, not for a global loggerImport Prisma in the project using
import prisma from "@/utils/prisma";
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't use TypeScript const enum.
Don't use the TypeScript directive @ts-ignore.
Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't use implicit any type on variable declarations.
Don't let variables evolve into any type through reassignments.
Don't use non-null assertions with the ! postfix operator.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use user-defined types.
Use as const instead of literal types and type annotations.
Use export type for types.
Use import type for types.
Don't declare empty interfaces.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Don't use TypeScript namespaces.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use parameter properties in class constructors.
Use either T[] or Array consistently.
Initialize each enum member value explicitly.
Make sure all enum members are literal values.
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
apps/web/utils/actions/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/server-actions.mdc)
apps/web/utils/actions/*.ts: Implement all server actions using thenext-safe-actionlibrary for type safety, input validation, context management, and error handling. Refer toapps/web/utils/actions/safe-action.tsfor client definitions (actionClient,actionClientUser,adminActionClient).
UseactionClientUserwhen only authenticated user context (userId) is needed.
UseactionClientwhen both authenticated user context and a specificemailAccountIdare needed. TheemailAccountIdmust be bound when calling the action from the client.
UseadminActionClientfor actions restricted to admin users.
Access necessary context (likeuserId,emailAccountId, etc.) provided by the safe action client via thectxobject in the.action()handler.
Server Actions are strictly for mutations (operations that change data, e.g., creating, updating, deleting). Do NOT use Server Actions for data fetching (GET operations). For data fetching, use dedicated GET API Routes combined with SWR Hooks.
UseSafeErrorfor expected/handled errors within actions if needed.next-safe-actionprovides centralized error handling.
Use the.metadata({ name: "actionName" })method to provide a meaningful name for monitoring. Sentry instrumentation is automatically applied viawithServerActionInstrumentationwithin the safe action clients.
If an action modifies data displayed elsewhere, userevalidatePathorrevalidateTagfromnext/cachewithin the action handler as needed.Server action files must start with
use server
Files:
apps/web/utils/actions/reply-tracking.ts
apps/web/utils/**
📄 CodeRabbit Inference Engine (.cursor/rules/project-structure.mdc)
Create utility functions in
utils/folder for reusable logic
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
apps/web/utils/**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/project-structure.mdc)
apps/web/utils/**/*.ts: Use lodash utilities for common operations (arrays, objects, strings)
Import specific lodash functions to minimize bundle size
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/ultracite.mdc)
**/*.{js,jsx,ts,tsx}: Don't useelements in Next.js projects.
Don't use elements in Next.js projects.
Don't use namespace imports.
Don't access namespace imports dynamically.
Don't use global eval().
Don't use console.
Don't use debugger.
Don't use var.
Don't use with statements in non-strict contexts.
Don't use the arguments object.
Don't use consecutive spaces in regular expression literals.
Don't use the comma operator.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use useless this aliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names th...
Files:
apps/web/utils/actions/reply-tracking.tsapps/web/utils/outlook/label.ts
🧠 Learnings (4)
📚 Learning: 2025-07-18T15:04:30.467Z
Learnt from: CR
PR: elie222/inbox-zero#0
File: apps/web/CLAUDE.md:0-0
Timestamp: 2025-07-18T15:04:30.467Z
Learning: Applies to apps/web/app/api/**/route.ts : Use `withEmailAccount` for email-account-level operations
Applied to files:
apps/web/utils/actions/reply-tracking.ts
📚 Learning: 2025-07-08T13:14:07.449Z
Learnt from: elie222
PR: elie222/inbox-zero#537
File: apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx:30-34
Timestamp: 2025-07-08T13:14:07.449Z
Learning: The clean onboarding page in apps/web/app/(app)/[emailAccountId]/clean/onboarding/page.tsx is intentionally Gmail-specific and should show an error for non-Google email accounts rather than attempting to support multiple providers.
Applied to files:
apps/web/utils/actions/reply-tracking.ts
📚 Learning: 2025-07-18T17:27:58.249Z
Learnt from: CR
PR: elie222/inbox-zero#0
File: .cursor/rules/server-actions.mdc:0-0
Timestamp: 2025-07-18T17:27:58.249Z
Learning: Applies to apps/web/utils/actions/*.ts : Use `actionClient` when both authenticated user context and a specific `emailAccountId` are needed. The `emailAccountId` must be bound when calling the action from the client.
Applied to files:
apps/web/utils/actions/reply-tracking.ts
📚 Learning: 2025-07-18T17:27:58.249Z
Learnt from: CR
PR: elie222/inbox-zero#0
File: .cursor/rules/server-actions.mdc:0-0
Timestamp: 2025-07-18T17:27:58.249Z
Learning: Applies to apps/web/utils/actions/*.ts : Access necessary context (like `userId`, `emailAccountId`, etc.) provided by the safe action client via the `ctx` object in the `.action()` handler.
Applied to files:
apps/web/utils/actions/reply-tracking.ts
🧬 Code Graph Analysis (1)
apps/web/utils/outlook/label.ts (3)
apps/web/utils/outlook/client.ts (1)
OutlookClient(19-63)packages/tinybird/src/publish.ts (1)
TinybirdEmailAction(12-12)apps/web/utils/outlook/message.ts (1)
getOrCreateFolderByName(56-136)
🔇 Additional comments (14)
apps/web/prisma/migrations/20250811130806_add_move_folder_action/migration.sql (2)
2-2: Enum change is irreversible; confirm migration execution strategyALTER TYPE ... ADD VALUE is a one-way change. Ensure this is intentional and that your environments won’t need a rollback. Also, on some Postgres versions this cannot run inside a transaction—confirm your migration runner executes this safely (Prisma typically handles this, but double-check your infra/version).
Would you like me to provide a reversible pattern (recreate type + cast) if rollback support is required?
14-14: Dropping VerificationToken.id default is safe given Prisma’s @default(cuid())
The Prisma schema already definesmodel VerificationToken { id String @id @default(cuid()) // … }so every insert from the application supplies its own
id. We didn’t find any raw‐SQL inserts or sequence usages on this table, and there is no orphaned sequence to clean up.apps/web/utils/outlook/label.ts (12)
4-4: LGTM: Clean import addition for folder resolution.The import of
getOrCreateFolderByNameis correctly added to support the dynamic folder destination feature.
237-237: LGTM: Function signature properly updated for dynamic folder support.The addition of the
folderNameparameter with a sensible default of "archive" maintains backward compatibility while enabling the new folder-based archiving functionality.Also applies to: 243-243
245-246: LGTM: Proper folder resolution implementation.The use of
getOrCreateFolderByNamecorrectly resolves the folder name to a folder ID that can be used with the Graph API move operations. This addresses the core requirement for dynamic folder destinations.
265-265: LGTM: Correctly using resolved folder ID for move operation.The move operation now properly uses
destinationFolderIdinstead of a hardcoded value or the folder name directly. This resolves the bug mentioned in the past review comments.
268-268: LGTM: Consistent logging with resolved folder ID.The log message correctly references
destinationFolderIdwhich provides more precise debugging information than the folder name.
296-299: LGTM: Improved error logging with dynamic folder reference.The error logging now properly uses the
folderNameparameter to provide contextual information about which folder the operation was attempting to move to.
304-304: LGTM: Consistent publish error logging.The publish error message correctly references the dynamic
folderNamefor better debugging context.
341-341: LGTM: Fallback path also uses resolved folder ID.The fallback move operation correctly uses
destinationFolderId, ensuring consistency between the primary and fallback code paths. This addresses the bug mentioned in the past review comments.
345-345: LGTM: Consistent error logging in fallback path.The fallback error logging properly uses
folderNamefor contextual information, maintaining consistency with the primary code path.
360-360: LGTM: Final fallback also uses resolved folder ID.Even the last-resort fallback (treating threadId as messageId) correctly uses
destinationFolderId, ensuring complete consistency across all code paths.
373-380: LGTM: Enhanced publish error logging.The publish error logging in the fallback path is comprehensive and uses the dynamic
folderNamefor better debugging context.
385-385: LGTM: Final error logging uses dynamic folder reference.The final error logging correctly uses
folderNameto provide contextual information about the failed operation.
| ALTER TABLE "Action" ADD COLUMN "folderName" TEXT; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "ExecutedAction" ADD COLUMN "folderName" TEXT; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "ScheduledAction" ADD COLUMN "folderName" TEXT; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce data integrity for MOVE_FOLDER actions
folderName is nullable, which is correct for non-MOVE_FOLDER actions. To prevent invalid rows, add CHECK constraints requiring folderName when type='MOVE_FOLDER' (and optionally disallowing it otherwise). This catches issues at the DB boundary and simplifies downstream assumptions.
Proposed follow-up migration (apply at the DB level):
ALTER TABLE "Action"
ADD CONSTRAINT action_move_folder_requires_name
CHECK (("type" <> 'MOVE_FOLDER') OR ("folderName" IS NOT NULL AND length(trim("folderName")) > 0));
ALTER TABLE "ExecutedAction"
ADD CONSTRAINT executed_action_move_folder_requires_name
CHECK (("type" <> 'MOVE_FOLDER') OR ("folderName" IS NOT NULL AND length(trim("folderName")) > 0));
ALTER TABLE "ScheduledAction"
ADD CONSTRAINT scheduled_action_move_folder_requires_name
CHECK (("type" <> 'MOVE_FOLDER') OR ("folderName" IS NOT NULL AND length(trim("folderName")) > 0));Optional: If Outlook mapping relies on stable IDs, consider storing provider folder IDs instead of names (or alongside names) to survive renames and localization.
🤖 Prompt for AI Agents
In
apps/web/prisma/migrations/20250811130806_add_move_folder_action/migration.sql
around lines 5 to 11, the new folderName columns are nullable which allows
invalid rows for MOVE_FOLDER actions; add DB CHECK constraints on Action,
ExecutedAction, and ScheduledAction that require folderName to be non-null and
non-empty (e.g., trimmed length > 0) when type = 'MOVE_FOLDER' (and optionally
enforce folderName IS NULL when type <> 'MOVE_FOLDER' if desired), and implement
these constraints as a follow-up migration so the DB enforces the rule;
optionally consider storing provider folder IDs (or both ID+name) instead of
just names for stability across renames/localization.
| if (emailAccount?.account?.provider !== "google") { | ||
| return { success: true }; | ||
| } |
There was a problem hiding this comment.
Provider gate swallows “account not found” — reorder checks
As written, a missing account (emailAccount === null) will hit the early “not google” return and silently succeed, bypassing the intended SafeError. Reorder the null check before the provider gate.
Apply:
- if (emailAccount?.account?.provider !== "google") {
- return { success: true };
- }
-
- if (!emailAccount) throw new SafeError("Email account not found");
+ if (!emailAccount) {
+ throw new SafeError("Email account not found");
+ }
+ if (emailAccount.account?.provider !== "google") {
+ return { success: true };
+ }Also applies to: 49-49
🤖 Prompt for AI Agents
In apps/web/utils/actions/reply-tracking.ts around lines 45-49, the current
provider check runs before verifying emailAccount exists, causing a missing
account (null) to hit the "not google" early return and silently succeed; move
the null/undefined check for emailAccount (and/or emailAccount.account) before
any provider gating and ensure you throw or return the intended SafeError when
the account is missing instead of returning success, then retain the provider
check afterward so non-Google providers still return { success: true }.
1 participant
Summary by CodeRabbit
New Features
Improvements
Migrations