Conversation
…s file and updating its usage in survey.ts and persona.ts. Enhance USER_ROLES with icons for UI display.
|
@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. |
|
Caution Review failedThe pull request is closed. WalkthroughRefactors the email provider architecture into provider-specific modules with shared types and a factory, adds Outlook support, introduces a new email-account–scoped onboarding flow and components, removes legacy onboarding, adds AI persona analysis and reply-context collection wired into draft generation, introduces a batch executed-rules API and UI merge, updates digest rendering and settings, and performs numerous type-import/path moves and tests additions. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ProcessRules.tsx
participant SWR as SWR
participant API as /api/user/executed-rules/batch
participant DB as Prisma
UI->>UI: gather messageIdsToFetch
UI->>SWR: request existingRules for messageIds
SWR->>API: GET ?messageIds=...
API->>DB: query executedRule by emailAccountId + messageIds
DB-->>API: return rules
API-->>SWR: { rulesMap }
SWR-->>UI: existingRules
UI->>UI: merge existingRules with runtime results -> allResults
UI->>UI: skip processing messages present in allResults
sequenceDiagram
participant Generator as generateDraftContent
participant Redis as Redis
participant AICTX as aiCollectReplyContext
participant KB as aiExtractRelevantKnowledge
participant Draft as aiDraftWithKnowledge
Generator->>Redis: getReply(threadId)
alt cached reply exists
Redis-->>Generator: cachedReply
Generator-->>Generator: return cachedReply
else no cache
par
Generator->>AICTX: collect reply context
Generator->>KB: extract knowledge
end
AICTX-->>Generator: emailHistoryContext
KB-->>Generator: knowledgeResult
Generator->>Draft: aiDraftWithKnowledge(knowledgeResult, emailHistoryContext)
Draft-->>Generator: reply
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
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: 26
🔭 Outside diff range comments (26)
apps/web/components/editor/extensions/LabelMention.tsx (3)
5-5: Import EmailLabel from the new shared types moduleTo align with the new provider contract and avoid tight coupling to provider implementations, import EmailLabel from "@/utils/email/types" instead of "@/providers/EmailProvider".
-import type { EmailLabel } from "@/providers/EmailProvider"; +import type { EmailLabel } from "@/utils/email/types";
75-85: Trim non-contract fields from suggestion itemsThese fields aren't part of the shared EmailLabel contract in "@/utils/email/types" and will cause type drift once the import is updated.
return [ ...filteredLabels, { id: `__create_new__${query.toLowerCase()}`, name: query, - gmailLabelId: undefined, - enabled: true, isCreateNew: true, }, ];
103-105: Remove console usage per guidelinesConsole logging is disallowed across TS/TSX. Either remove logs or route through an allowed client-side telemetry mechanism. For now, remove the console calls to comply.
- console.warn("Error during mention cleanup:", error); + // noop- console.error("Error during mention start:", error); + // noop- console.error("Error during mention update:", error); + // noop- console.error("Error during mention keydown:", error); + // noopIf you prefer telemetry, I can wire this to your client error reporter instead.
Also applies to: 127-129, 151-154, 166-169
apps/web/app/(app)/[emailAccountId]/settings/DigestScheduleForm.tsx (1)
1-1: Missing "use client" directiveThis component consumes client-only hooks. Add "use client" to avoid Next.js app router SSR errors.
+"use client";apps/web/app/api/threads/[id]/route.ts (2)
41-55: Remove local try/catch in GET; rely on centralized error handlingPer guidelines, don't use try/catch in GET routes when using authentication middleware. Let middleware handle errors and formatting.
const includeDrafts = searchParams.get("includeDrafts") === "true"; - - 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);
31-31: Use withEmailAccount for email-account-level GET routeThe
withEmailProvidermiddleware does not wrap or composewithEmailAccount—it invokes a different middleware (emailProviderMiddleware). For routes operating on a specific email account (likeapps/web/app/api/threads/[id]/route.ts), we must usewithEmailAccountper org guidelines.
- File: apps/web/app/api/threads/[id]/route.ts
- Line: 31
Suggested fix:
-export const GET = withEmailProvider(async (request, context) => { +export const GET = withEmailAccount(async (request, context) => {apps/web/utils/ai/actions.ts (1)
263-266: Avoid any in TypeScript; tighten digest args typingReplace ActionFunction with a concrete args type.
-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/assistant/process-assistant-email.ts (3)
92-94: Guard missing In-Reply-To before fetching original messageoriginalMessageId can be undefined; provider.getOriginalMessage(undefined) will likely throw.
- 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;
95-131: Scope DB query by emailAccountId, not emailUse the provided emailAccountId to avoid mismatches and ensure proper ownership scoping.
- const [emailAccount, executedRule, senderCategory] = await Promise.all([ - prisma.emailAccount.findUnique({ - where: { email: userEmail }, + const [emailAccount, executedRule, senderCategory] = await Promise.all([ + prisma.emailAccount.findUnique({ + where: { id: emailAccountId }, select: { id: true, userId: true, email: true, about: true, user: { select: { aiProvider: true, aiModel: true, aiApiKey: true, }, }, rules: { include: { actions: true, categoryFilters: true, group: { select: { id: true, name: true, items: { select: { id: true, type: true, value: true, }, }, }, }, }, }, categories: true, }, }),
26-33: Fix label handling: apply both labels and remove processing label using threadId (not messageId)Current code:
- Only applies the first label (assistant never applied).
- Calls removeThreadLabel with a messageId (bug).
Pass both messageId and threadId to withProcessingLabels, apply both labels, and remove processing label at thread level.
- return withProcessingLabels(message.id, provider, () => + return withProcessingLabels(message.id, message.threadId, provider, () => processAssistantEmailInternal({ emailAccountId, userEmail, message, provider, }), );-async function withProcessingLabels<T>( - messageId: string, - provider: EmailProvider, - fn: () => Promise<T>, -): Promise<T> { +async function withProcessingLabels<T>( + messageId: string, + threadId: string, + provider: EmailProvider, + fn: () => Promise<T>, +): Promise<T> {- const labels = results + const labels = results .map((result) => result.status === "fulfilled" ? result.value?.id : undefined, ) .filter(isDefined); if (labels.length) { - // Fire and forget the initial labeling - provider.labelMessage(messageId, labels[0]).catch((error) => { + // Fire and forget: apply both labels + Promise.all( + labels.map((labelId) => provider.labelMessage(messageId, labelId)), + ).catch((error) => { logger.error("Error labeling message", { error }); }); }- const processingLabel = results[0]; + const processingLabel = results[0]; const processingLabelId = processingLabel.status === "fulfilled" ? processingLabel.value?.id : undefined; if (processingLabelId) { - await provider - .removeThreadLabel(messageId, processingLabelId) + await provider + .removeThreadLabel(threadId, processingLabelId) .catch((error) => { logger.error("Error removing processing label", { error }); }); }Also applies to: 242-247, 268-279, 290-296
apps/web/app/api/watch/controller.ts (1)
24-31: Fix Microsoft provider name mismatch in watch controller and messages routeThe Outlook/email Graph provider uses the literal name
"microsoft-entra-id", so any checks against"microsoft"will never match. This causes:
- The
watchEmailsSubscriptionIdto staynull(unwatch can’t clean up subscriptions).- The Microsoft branch in
messages/route.tsto never execute (no messages fetched).Please update both locations:
• apps/web/app/api/watch/controller.ts (around line 29)
• apps/web/app/api/messages/route.ts (lines 54 & 65)Suggested diffs:
--- a/apps/web/app/api/watch/controller.ts +++ b/apps/web/app/api/watch/controller.ts @@ -27,7 +27,7 @@ export async function POST(req: NextRequest) { data: { watchEmailsExpirationDate: result.expirationDate, - watchEmailsSubscriptionId: - provider.name === "microsoft" ? result.subscriptionId : null, + watchEmailsSubscriptionId: result.subscriptionId ?? null, }, });--- a/apps/web/app/api/messages/route.ts +++ b/apps/web/app/api/messages/route.ts @@ 54,7 @@ export async function GET(request: Request) { if (emailProvider.name === "google") { // ... - } else if (emailProvider.name === "microsoft") { + } else if (emailProvider.name === "microsoft-entra-id") { // fetch messages via Microsoft Graph } else { return NextResponse.json({ error: "unsupported provider" }, { status: 400 });After these updates, Microsoft subscriptions and message fetching will work as intended.
apps/web/app/api/threads/route.ts (1)
39-52: Wrap your GET handler withwithErrorand remove the local try/catchThe
withEmailProvidermiddleware only adds authentication—errors still need to be handled centrally. To rely on our global error middleware:
- In apps/web/app/api/threads/route.ts:
- Remove the
try { … } catch { … }block aroundgetThreads(lines 39–52).- Define a standalone handler and export it wrapped with both
withErrorandwithEmailProvider. For example:- export async function GET(request: RequestWithEmailProvider) { - try { - const threads = await getThreads({ query, emailAccountId, emailProvider }); - return NextResponse.json(threads); - } catch (error) { - logger.error("Error fetching threads", { error, emailAccountId }); - return NextResponse.json({ error: "Failed to fetch threads" }, { status: 500 }); - } - } + async function GETHandler(request: RequestWithEmailProvider) { + const threads = await getThreads({ query, emailAccountId, emailProvider }); + return NextResponse.json(threads); + } + + export const GET = withEmailProvider(withError(GETHandler));This ensures any errors bubble up to our centralized error handler instead of being caught in each route.
apps/web/app/api/user/stats/newsletters/helpers.ts (1)
81-103: Move provider-specific filter logic out of API helpersThis helper hardcodes Gmail/Outlook semantics (uses GmailLabel and Outlook “INBOX” assumptions). To keep provider-specific details isolated, push this logic into the provider layer (e.g., Gmail/Outlook utils) and expose a single provider-level capability (e.g., provider.isAutoArchiveFilter(filter)).
- Improves separation of concerns and testability
- Keeps API helpers provider-agnostic
apps/web/__tests__/ai/choose-rule/draft-management.test.ts (1)
3-3: Use the shared Prisma mock for consistencyPer testing conventions, import the provided Prisma mock and mock "@/utils/prisma" rather than crafting a bespoke mock in this file.
- import prisma from "@/utils/prisma"; + import prisma from "@/utils/__mocks__/prisma"; @@ -vi.mock("@/utils/prisma", () => ({ - default: { - executedAction: { - findFirst: vi.fn(), - update: vi.fn(), - }, - }, -})); +vi.mock("@/utils/prisma");This keeps mocks consistent across the suite and avoids drift.
Also applies to: 9-16
apps/web/app/api/user/stats/newsletters/route.ts (2)
40-68: Harden getTypeFilters against undefined inputDue to the Zod transform, NewsletterStatsQuery["types"] can be undefined. Add a sane default to prevent runtime errors.
-function getTypeFilters(types: NewsletterStatsQuery["types"]) { +function getTypeFilters(types: NewsletterStatsQuery["types"] = []) { const typeMap = Object.fromEntries(types.map((type) => [type, true]));
189-206: Avoid $queryRawUnsafe; prefer parameterized $queryRaw with Prisma.sqlCurrent query builds dynamic SQL and uses $queryRawUnsafe. While inputs are validated, switching to $queryRaw with parameter interpolation improves safety and maintainability.
- Replace manual $ placeholders with template parameters: to_timestamp(${value}::double precision)
- Use Prisma.sql fragments and Prisma.join to build WHERE conditions
- Keep orderBy limited to a z.enum; LIMIT uses a number from Zod
Example structure (illustrative):
const whereParts: Prisma.Sql[] = []; if (options.fromDate) { whereParts.push(Prisma.sql`"date" >= to_timestamp(${options.fromDate / 1000}::double precision)`); } if (options.toDate) { whereParts.push(Prisma.sql`"date" <= to_timestamp(${options.toDate / 1000}::double precision)`); } if (options.read) whereParts.push(Prisma.sql`read = true`); else if (options.unread) whereParts.push(Prisma.sql`read = false`); if (options.unarchived) whereParts.push(Prisma.sql`inbox = true`); else if (options.archived) whereParts.push(Prisma.sql`inbox = false`); whereParts.push(Prisma.sql`"emailAccountId" = ${options.emailAccountId}`); const whereSql = whereParts.length ? Prisma.sql`WHERE ${Prisma.join(whereParts, Prisma.sql` AND `)}` : Prisma.sql``; const orderSql = Prisma.sql([getOrderByClause(options.orderBy ?? "emails")]); // enum-validated const limitSql = options.limit ? Prisma.sql`LIMIT ${options.limit}` : Prisma.sql``; const query = Prisma.sql` WITH email_message_stats AS ( SELECT "from", COUNT(*)::int as "count", SUM(CASE WHEN inbox = true THEN 1 ELSE 0 END)::int as "inboxEmails", SUM(CASE WHEN read = true THEN 1 ELSE 0 END)::int as "readEmails", MAX("unsubscribeLink") as "unsubscribeLink" FROM "EmailMessage" ${whereSql} GROUP BY "from" ) SELECT * FROM email_message_stats ORDER BY ${Prisma.raw(getOrderByClause(options.orderBy ?? "emails"))} ${limitSql} `; const results = await prisma.$queryRaw<NewsletterCountRawResult[]>(query);Even if you keep the current approach, at minimum drop the redundant spread of query.values in $queryRawUnsafe call and ensure all variable pieces remain Zod-validated.
Also applies to: 207-214
apps/web/utils/ai/choose-rule/match-rules.ts (1)
305-314: Avoid relying on implicit string coercion for possibly undefined headersDefault headers to empty strings to keep intent clear and satisfy strict null checks.
- const fromMatch = from - ? safeRegexTest(from, message.headers.from, true) - : true; - const toMatch = to ? safeRegexTest(to, message.headers.to, true) : 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) + ? safeRegexTest(subject, message.headers.subject || "", false) : true; const bodyMatch = body ? safeRegexTest(body, message.textPlain || "", false) : true;apps/web/utils/cold-email/is-cold-email.ts (2)
162-171: Bug: Prisma findUnique used with non-unique filters (will throw at runtime)findUnique can only filter by a unique identifier. Adding status is invalid. Use findFirst with a proper where clause.
Apply:
- 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 }, + });
88-153: Deduplicate isColdEmail and isColdEmailWithProviderBoth functions contain nearly identical logic; keep one source of truth to reduce drift and improve maintainability.
Suggested consolidation:
-export async function isColdEmailWithProvider({...}): Promise<...> { - // duplicate of isColdEmail -} +export async function isColdEmailWithProvider(args: { + email: EmailForLLM & { threadId?: string }; + emailAccount: Pick<EmailAccount, "coldEmailPrompt"> & EmailAccountWithAI; + provider: EmailProvider; + modelType: ModelType; +}): Promise<{ + isColdEmail: boolean; + reason: ColdEmailBlockerReason; + aiReason?: string | null; +}> { + return isColdEmail(args); +}apps/web/utils/middleware.ts (2)
198-211: Bug: Prisma findUnique with extra filters is invalidfindUnique should only use a unique field. You're adding userId, which will be ignored or cause type errors. Use findFirst to ensure ownership scoping.
Apply:
- const emailAccount = await prisma.emailAccount.findUnique({ - where: { - id: emailAccountId, - userId, // ensure it belongs to the user - }, + const emailAccount = await prisma.emailAccount.findFirst({ + where: { + id: emailAccountId, + userId, // ensure it belongs to the user + }, include: { account: { select: { provider: true, }, }, }, });
116-120: Remove console usage; use logger onlyGuidelines prohibit console in repo code. You already log below; remove this dev-only console block.
Apply:
- if (env.NODE_ENV === "development") { - // biome-ignore lint/suspicious/noConsole: helpful for debugging - console.error(error); - } + // Optional: rely solely on scoped logger for consistencyapps/web/utils/assess.ts (1)
103-111: Avoid accessing provider internals with (client as any).clientThis breaks the provider abstraction and violates provider-specific isolation. Add explicit methods to the provider interface or helper utilities in the gmail module to fetch filters/forwarding addresses.
Proposed approaches:
- Add methods to GmailProvider (and optionally to EmailProvider as no-op/default): getFilters(), getForwardingAddresses()
- Or create gmail-only helpers (e.g., utils/gmail/settings.ts) and call them with a typed GmailProvider, avoiding any casts.
Also applies to: 113-127
apps/web/__tests__/helpers.ts (1)
4-18: Fix the overrides handling in getEmailAccount.The function should spread all overrides, not just the email field. The current implementation ignores other override fields.
Apply this diff to properly handle all overrides:
export function getEmailAccount( overrides: Partial<EmailAccountWithAI> = {}, ): EmailAccountWithAI { return { 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, }, + ...overrides, }; }apps/web/utils/reply-tracker/generate-draft.ts (2)
115-120: Add error handling for Redis operationsThe Redis
getReplyoperation should handle potential errors gracefully to avoid crashes.// Check Redis cache for reply -const reply = await getReply({ - emailAccountId: emailAccount.id, - messageId: lastMessage.id, -}); +let reply; +try { + reply = await getReply({ + emailAccountId: emailAccount.id, + messageId: lastMessage.id, + }); +} catch (error) { + logger.error("Failed to get cached reply", { error }); + // Continue without cache +}
195-201: Add error handling for Redis save operationThe Redis
saveReplyoperation should handle errors gracefully.if (typeof text === "string") { - await saveReply({ - emailAccountId: emailAccount.id, - messageId: lastMessage.id, - reply: text, - }); + try { + await saveReply({ + emailAccountId: emailAccount.id, + messageId: lastMessage.id, + reply: text, + }); + } catch (error) { + logger.error("Failed to save reply to cache", { error }); + // Continue - caching failure shouldn't break the flow + } }apps/web/utils/email/provider.ts (1)
9-15: Consider using a union type for provider parameterInstead of
string | null, use a specific union type to ensure type safety at compile time.+type SupportedProvider = "google" | "microsoft"; + export async function createEmailProvider({ emailAccountId, provider, }: { emailAccountId: string; - provider: string | null; + provider: SupportedProvider | null; }): Promise<EmailProvider> {
🧹 Nitpick comments (33)
apps/web/app/(app)/(redirects)/onboarding/page.tsx (1)
3-5: Prefer returning the redirect helper (drop async/await) for clearer intent and better type inferenceReturning the redirect call communicates non-returning control flow and aligns with Next’s redirect typing (never). It also avoids an unnecessary async function.
-export default async function OnboardingPage() { - await redirectToEmailAccountPath("/onboarding"); -} +export default function OnboardingPage() { + return redirectToEmailAccountPath("/onboarding"); +}apps/web/components/editor/extensions/MentionList.tsx (1)
85-87: Minor: consider design-system consistency for emphasisUsing is fine; if the design system prefers utility classes, you could swap to a span with font-semibold for consistency.
- <strong>Create label:</strong>{" "} + <span className="font-semibold">Create label:</span>{" "}apps/web/components/AccountSwitcher.tsx (1)
116-139: Navigation via hard reload: add preventDefault and consider replace()Intentional full reload is clear. Minor improvements:
- Call preventDefault in onSelect to avoid any default item behaviors before redirect.
- Consider window.location.replace(...) to avoid polluting history if “back” should not return to the previous account view.
- onSelect={() => { + onSelect={(e) => { + e.preventDefault(); // 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); + window.location.replace(getHref(emailAccount.id)); }}Confirm desired back-button behavior for account switching (keep vs. replace history).
apps/web/utils/llms/model.ts (1)
43-44: Early-return bypasses economy/chat for user-supplied API keysWith this change, modelType is ignored whenever aiApiKey is present. Confirm this is intended product behavior; otherwise, consider honoring economy/chat selections by delegating to selectEconomyModel/selectChatModel when requested.
Would you like a change that respects modelType for user keys while still using userAi.aiProvider/aiModel when provided?
- if (userAi.aiApiKey) return selectDefaultModel(userAi); + if (userAi.aiApiKey) { + switch (modelType) { + case "economy": + return selectEconomyModel(userAi); + case "chat": + return selectChatModel(userAi); + default: + return selectDefaultModel(userAi); + } + }apps/web/components/SideNavWithTopNav.tsx (1)
1-3: Onboarding layout short-circuit is fine; consider routing-level layoutBypassing the shell for /[emailAccountId]/onboarding is clear. If this persists, consider moving this concern to a route-specific layout.tsx to avoid path parsing inside a generic layout component.
Also applies to: 22-30
apps/web/app/(app)/[emailAccountId]/settings/DigestItemsForm.tsx (2)
21-25: Make showSaveButton optional with a sensible defaultTo reduce call-site churn and improve ergonomics, default showSaveButton to true and make it optional.
-export function DigestItemsForm({ - showSaveButton, -}: { - showSaveButton: boolean; -}) { +export function DigestItemsForm({ + showSaveButton = true, +}: { + showSaveButton?: boolean; +}) {
154-154: Styling nitbg-background addition improves contrast with the page; keep it. If you want tighter visual grouping, consider gap-3 and py-3, but current values are fine.
apps/web/app/(app)/[emailAccountId]/settings/DigestScheduleForm.tsx (2)
95-103: Type mutate using SWR’s KeyedMutator for accuracyCurrent type mutate: () => void loses mutate’s call signature. Tighten it to KeyedMutator.
-function DigestScheduleFormInner({ +function DigestScheduleFormInner({ data, mutate, showSaveButton, }: { data: GetDigestScheduleResponse | undefined; - mutate: () => void; + mutate: import("swr").KeyedMutator<GetDigestScheduleResponse>; showSaveButton: boolean; }) {
185-185: Make grid responsive for mobile-firstThree columns can be cramped on small screens. Use responsive columns.
- <div className="grid grid-cols-3 gap-2"> + <div className="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-3 gap-2">apps/web/components/ScrollableFadeContainer.tsx (1)
17-31: Consider orientation support (horizontal fades) and displayNameIf this is used for horizontal scroll (e.g., role cards), add an orientation prop to render left/right fades. Also set displayName for devtools clarity.
Example API:
interface ScrollableFadeContainerProps { children: ReactNode; className?: string; - height?: string; + height?: string; + orientation?: "vertical" | "horizontal"; showTopFade?: boolean; showBottomFade?: boolean; fadeHeight?: string; fadeFromClass?: string; } -export const ScrollableFadeContainer = forwardRef<...>(function ScrollableFadeContainer( +export const ScrollableFadeContainer = forwardRef<...>(function ScrollableFadeContainer( { children, className, height = "h-[500px]", + orientation = "vertical", showTopFade = true, showBottomFade = true, fadeHeight = "h-8", fadeFromClass = "from-background", }, ref, ) { return ( <div className="relative"> - {showTopFade && ( + {orientation === "vertical" && showTopFade && ( <div className={cn( "absolute top-0 left-0 right-0 bg-gradient-to-b to-transparent z-10 pointer-events-none", fadeHeight, fadeFromClass, )} /> )} - <ScrollArea className={cn(height, "pr-1.5")}> + <ScrollArea className={cn(height, "pr-1.5")}> <div ref={ref} className={className}> {children} </div> </ScrollArea> - {showBottomFade && ( + {orientation === "vertical" && showBottomFade && ( <div className={cn( "absolute bottom-0 left-0 right-0 bg-gradient-to-t to-transparent z-10 pointer-events-none", fadeHeight, fadeFromClass, )} /> )} </div> ); }); + +ScrollableFadeContainer.displayName = "ScrollableFadeContainer";If StepWho needs horizontal fades, I can extend this to render left/right gradients similarly.
Also applies to: 33-60
apps/web/utils/email/types.ts (2)
25-34: Tighten return types and align with guidelines
- Replace Promise with concrete types (e.g., created filter or provider-specific metadata). This improves DX and safety.
- Org style prefers export type for type exports. Converting interfaces to type aliases is optional here but would align style-wide.
I can draft precise return types if you point me to the Gmail/Outlook implementations.
Also applies to: 36-160
12-23: Confirm EmailLabel fields cover client usageDownstream code (LabelMention) currently uses fields like gmailLabelId and enabled. If those are provider-specific, keep EmailLabel minimal and have provider-specific extensions, or add optional fields with a comment noting provider scope.
I can help refactor client code to only rely on id/name and hide provider-specific fields behind mappers.
apps/web/app/api/threads/[id]/route.ts (1)
7-7: Validate param format/lengthEnforce non-empty id per “validate all parameters” guideline.
-const threadQuery = z.object({ id: z.string() }); +const threadQuery = z.object({ id: z.string().min(1) });apps/web/utils/ai/actions.ts (1)
33-39: Avoid logging full action payload at trace levelThis may include sensitive content. Log minimal fields.
- logger.trace("Running action:", action); + logger.trace("Running action detail", { + id: action.id, + hasContent: Boolean((action as any).content), + });apps/web/utils/assistant/process-assistant-email.ts (1)
167-170: Clarify log messageThis is the emailAccount fetch. Message should say “Email account not found”.
- logger.error("User not found", loggerOptions); + logger.error("Email account not found", loggerOptions);apps/web/utils/reply-tracker/check-sender-reply-history.ts (1)
21-25: Comment text contradicts behavior; align wording with safe defaultsCode returns hasReplied: true on invalid email or errors (safe default: assume already replied). Update comments accordingly.
- logger.warn("Could not extract email from sender", { senderEmail }); - // Default to assuming a reply might be needed if email is invalid + logger.warn("Could not extract email from sender", { senderEmail }); + // Default to assuming a reply was already sent (avoid unintended automations) return { hasReplied: true, receivedCount: 0 };- // Catch potential errors from Promise.all or other unexpected issues + // 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 + // Default to assuming a reply was already sent (avoid unintended automations) return { hasReplied: true, receivedCount: 0 };Also applies to: 49-57
apps/web/utils/categorize/senders/categorize.ts (1)
64-105: Reduce duplication: consolidate Gmail-specific and provider-based flowsYou now have both categorizeSender (Gmail-only) and categorizeSenderWithProvider (provider-agnostic) with near-identical logic. Consider deprecating the Gmail-specific version or routing it through the provider-based one to avoid drift.
Happy to provide a patch that keeps categorizeSender for backward compatibility but proxies to categorizeSenderWithProvider via a GmailProvider instance.
apps/web/utils/email/microsoft.ts (2)
52-53: Decouple provider from API route codeOutlook provider imports processOutlookHistory from an API route path (app/api/outlook/webhook/process-history). This couples utils with app router code and risks circular dependencies.
Move the history processing logic into a utils/outlook/history module and import it here and in the API route to keep provider-specific details under utils/.
508-517: Nit: avoid redundant base64 round-trip for attachmentsGraph returns contentBytes as base64. Re-encoding base64 to base64 is redundant. You can return contentBytes directly for performance.
Suggested simplification:
- const data = attachment.contentBytes - ? Buffer.from(attachment.contentBytes, "base64").toString("base64") - : ""; + const data = attachment.contentBytes || "";apps/web/utils/ai/choose-rule/execute.ts (1)
22-36: Add explicit return type to exported async functionFor clarity and to satisfy strict typing in AI code, annotate the return type.
-export async function executeAct({ +export async function executeAct({ client, executedRule, userEmail, userId, emailAccountId, message, }: { client: EmailProvider; executedRule: ExecutedRuleWithActionItems; message: ParsedMessage; userEmail: string; userId: string; emailAccountId: string; -}) { +}): Promise<void> {apps/web/app/api/user/stats/newsletters/route.ts (1)
120-127: Remove duplicate types NewsletterCountResult and NewsletterCountRawResultThey’re identical; consolidate to a single type to reduce drift.
-type NewsletterCountResult = { +type NewsletterCountResult = { from: string; count: number; inboxEmails: number; readEmails: number; unsubscribeLink: string | null; }; -type NewsletterCountRawResult = { - from: string; - count: number; - inboxEmails: number; - readEmails: number; - unsubscribeLink: string | null; -}; +type NewsletterCountRawResult = NewsletterCountResult;apps/web/utils/ai/choose-rule/match-rules.ts (1)
202-214: Add explicit return type to exported function for clarityConsider annotating the return of findMatchingRule for better DX.
-export async function findMatchingRule({ +export async function findMatchingRule({ rules, message, emailAccount, client, modelType, }: { rules: RuleWithActionsAndCategories[]; message: ParsedMessage; emailAccount: EmailAccountWithAI; client: EmailProvider; modelType: ModelType; -}) { +}): Promise<{ + rule?: RuleWithActionsAndCategories; + reason?: string; + matchReasons?: MatchReason[]; +}> {apps/web/utils/cold-email/is-cold-email.ts (1)
281-316: Provider side-effects should not break DB upsert pathIf labeling/archiving fails after DB upsert, the function throws and may leave the system in a partially-updated state. Consider catching provider-side errors, logging, and continuing (or returning a structured error), depending on product requirements.
Example:
- if (coldEmailLabel?.name) { - await provider.labelMessage(email.id, coldEmailLabel.name); - } + 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 (err) { + logger.error("Cold email provider ops failed", { + emailAccountId: emailAccount.id, + threadId: email.threadId, + error: err, + }); + // Optionally: swallow or rethrow based on desired behavior + }apps/web/app/api/messages/route.ts (1)
26-34: Redundant try/catch inside helper; rely on centralized error handlingGiven middleware centralizes error handling, consider removing the try/catch in getMessages and let errors bubble up. If you keep it for logging, ensure we avoid double-logging the same error.
Also applies to: 75-84
apps/web/utils/cold-email/is-cold-email.test.ts (1)
11-17: Prefer shared Prisma mock for consistency in VitestPer our testing learnings, import the shared Prisma mock from "@/utils/mocks/prisma" and mock "@/utils/prisma" accordingly to ensure consistent behavior across tests and easier maintenance.
Example:
-vi.mock("@/utils/prisma", () => ({ - default: { - coldEmail: { - upsert: vi.fn(), - }, - }, -})); +import prismaMock from "@/utils/__mocks__/prisma"; +vi.mock("@/utils/prisma", () => ({ default: prismaMock }));apps/web/utils/middleware.ts (1)
219-227: Optional: add provider name to logsAdding provider name to the context helps ops/debugging across providers.
- providerReq.emailProvider = provider; + providerReq.emailProvider = provider; + logger.info("Email provider initialized", { + emailAccountId, + userId, + provider: provider.name, + });apps/web/utils/assess.ts (1)
94-101: Magic numbers for default label countsExtract default label counts into shared constants (per provider) to avoid drift and improve clarity.
Example:
// utils/email/constants.ts export const DEFAULT_LABEL_COUNT = { google: 13, microsoft: 8, } as const;Then:
- const DEFAULT_LABEL_COUNT = 13; - return labels.length - DEFAULT_LABEL_COUNT; + return labels.length - DEFAULT_LABEL_COUNT.google;apps/web/utils/ai/choose-rule/choose-args.ts (1)
49-53: Logging around draft generation — good additionScoped, structured logs before/after and on error improve observability. Consider redacting or hashing PII (email) if logs are exported externally.
Also applies to: 60-63, 65-69
apps/web/utils/actions/onboarding.validation.ts (1)
3-5: Tighten validation to allowed roles and trim inputIf you have a USER_ROLES constant, prefer z.enum for strictness; also trim and handle “Other” if supported.
Example:
+import { USER_ROLES } from "@/utils/constants/user-roles"; +const ROLES = USER_ROLES.map((r) => r.value) as [string, ...string[]]; -export const stepWhoBody = z.object({ - role: z.string().min(1, "Please select your role."), -}); +export const stepWhoBody = z.object({ + role: z + .string() + .trim() + .min(1, "Please select your role.") + .refine((v) => ROLES.includes(v), "Please select a valid role."), +});If “other” is allowed with a custom string, model it as:
- role: z.enum([...ROLES, "other"])
- customRole: z.string().trim().min(1).optional().refine((v, ctx) => (ctx.parent.role !== "other" ? true : !!v), "Please specify your role.")
apps/web/app/(app)/[emailAccountId]/onboarding/StepIntro.tsx (1)
69-69: Consider responsive height for Benefit cardsThe fixed height of
h-[50vh]might cause layout issues on smaller screens or different viewport sizes.Consider using a min-height with responsive breakpoints:
-<CardBasic className="rounded-2xl shadow-none grid grid-cols-5 p-0 pl-4 pt-4 gap-8 h-[50vh]"> +<CardBasic className="rounded-2xl shadow-none grid grid-cols-5 p-0 pl-4 pt-4 gap-8 min-h-[300px] md:min-h-[400px] lg:min-h-[500px]">apps/web/app/(app)/[emailAccountId]/assistant/ProcessRules.tsx (1)
110-114: Consider POST request for large message ID listsWhen fetching many message IDs, the GET request URL could exceed browser/server limits. Consider using a POST request or chunking the IDs.
// Consider chunking if there are many IDs const CHUNK_SIZE = 50; const chunks = []; for (let i = 0; i < messageIdsToFetch.length; i += CHUNK_SIZE) { chunks.push(messageIdsToFetch.slice(i, i + CHUNK_SIZE)); } // Then fetch in batches and merge resultsapps/web/utils/ai/knowledge/persona.ts (1)
47-50: Consider adding explicit return type annotationWhile TypeScript can infer the return type, explicitly declaring it improves code readability and catches potential type mismatches earlier.
export async function aiAnalyzePersona(options: { emails: EmailForLLM[]; emailAccount: EmailAccountWithAI; -}): Promise<PersonaAnalysis | null> { +}): Promise<PersonaAnalysis | null> {apps/web/utils/email/provider.ts (1)
23-23: Improve error message with available providersThe error message could be more helpful by listing supported providers.
- throw new Error(`Unsupported provider: ${provider}`); + throw new Error(`Unsupported provider: ${provider}. Supported providers are: google, microsoft`);
| # cli logs | ||
| logs No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Anchor ignore to repo root to avoid over-ignoring nested paths named "logs".
Plain "logs" will ignore any file/dir named logs anywhere in the tree. If this is specifically for CLI logs at the repo root (per the comment), prefer a root-anchored directory pattern.
Apply:
-logs
+/logs/Optionally, if only log files under that dir should be ignored (keeping the dir itself), use:
+/logs/*
+!/logs/.gitkeepTo verify there aren’t other "logs" dirs you might accidentally ignore, run:
🏁 Script executed:
#!/bin/bash
# List any directories named exactly 'logs' anywhere in the repo
fd -t d -H '^logs$' || true
# List any files named exactly 'logs'
fd -t f -H '^logs$' || trueLength of output: 43
🏁 Script executed:
#!/bin/bash
# Force-search through ignored and hidden files for any 'logs' entries
fd -t d -H -I '^logs$' || true
fd -t f -H -I '^logs$' || trueLength of output: 60
Anchor the CLI logs ignore to the repo root
The only logs directory found (.git/logs/) lives inside Git’s own metadata and isn’t affected by .gitignore rules. To prevent future nested logs dirs from being ignored by accident, update lines 77–78 in .gitignore:
- logs
+ /logs/Optionally, if you want to ignore only the contents (but keep the directory and a .gitkeep), use:
+ /logs/*
+ !/logs/.gitkeep• File: .gitignore Lines 77–78
• Diff: see above
📝 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.
| # cli logs | |
| logs | |
| # cli logs | |
| /logs/ |
🤖 Prompt for AI Agents
In .gitignore around lines 77 to 78, the current rule "logs" can accidentally
match Git's internal .git/logs and other nested dirs; replace it with a rooted
pattern by changing the entry to "/logs" to ignore only a top-level logs
directory, or if you want to keep the directory and ignore only its contents
change it to "logs/*" (and add "logs/.gitkeep" to repo if you keep the dir).
| test( | ||
| "uses additional user context when available", | ||
| async () => { | ||
| const result = await aiAnalyzePersona({ | ||
| emails: getFounderEmails(), | ||
| emailAccount: getEmailAccount(), | ||
| }); | ||
|
|
||
| console.debug( | ||
| "Analysis with user context:\n", | ||
| JSON.stringify(result, null, 2), | ||
| ); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| expect(result?.industry.toLowerCase()).toContain("hr"); | ||
| expect(result?.confidence).toBe("high"); | ||
| }, | ||
| TIMEOUT, | ||
| ); |
There was a problem hiding this comment.
Fix test logic for user context validation.
The test expects the industry to contain "hr" and confidence to be "high", but no HR-related context is provided in the test data or email account. This test will likely fail.
test(
"uses additional user context when available",
async () => {
+ // Add user context that would indicate HR industry
+ const emailAccountWithContext = getEmailAccount({
+ about: "I work in HR managing employee communications and benefits"
+ });
+
const result = await aiAnalyzePersona({
emails: getFounderEmails(),
- emailAccount: getEmailAccount(),
+ emailAccount: emailAccountWithContext,
});
console.debug(
"Analysis with user context:\n",
JSON.stringify(result, null, 2),
);
expect(result).toBeDefined();
- expect(result?.industry.toLowerCase()).toContain("hr");
- expect(result?.confidence).toBe("high");
+ // Validate that user context influences the analysis
+ expect(result?.reasoning).toBeDefined();
+ expect(result?.confidence).toMatch(/medium|high/);
},
TIMEOUT,
);📝 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.
| test( | |
| "uses additional user context when available", | |
| async () => { | |
| const result = await aiAnalyzePersona({ | |
| emails: getFounderEmails(), | |
| emailAccount: getEmailAccount(), | |
| }); | |
| console.debug( | |
| "Analysis with user context:\n", | |
| JSON.stringify(result, null, 2), | |
| ); | |
| expect(result).toBeDefined(); | |
| expect(result?.industry.toLowerCase()).toContain("hr"); | |
| expect(result?.confidence).toBe("high"); | |
| }, | |
| TIMEOUT, | |
| ); | |
| test( | |
| "uses additional user context when available", | |
| async () => { | |
| // Add user context that would indicate HR industry | |
| const emailAccountWithContext = getEmailAccount({ | |
| about: "I work in HR managing employee communications and benefits", | |
| }); | |
| const result = await aiAnalyzePersona({ | |
| emails: getFounderEmails(), | |
| emailAccount: emailAccountWithContext, | |
| }); | |
| console.debug( | |
| "Analysis with user context:\n", | |
| JSON.stringify(result, null, 2), | |
| ); | |
| expect(result).toBeDefined(); | |
| // Validate that user context influences the analysis | |
| expect(result?.reasoning).toBeDefined(); | |
| expect(result?.confidence).toMatch(/medium|high/); | |
| }, | |
| TIMEOUT, | |
| ); |
🤖 Prompt for AI Agents
In apps/web/__tests__/ai-persona.test.ts around lines 205 to 223, the test
asserts the returned industry contains "hr" and confidence is "high" but the
provided test data (emails and emailAccount) contains no HR-related context, so
update the test data or the assertions: either inject explicit HR context into
the fixture(s) used (e.g., add HR-related text to getFounderEmails() or set
emailAccount.profile.industry = "HR" / include HR keywords in the mock account)
so the analyzer can return "hr" with high confidence, or relax the assertions to
match the actual expected output from the existing fixtures (e.g., check for a
different industry or confidence level); ensure the chosen fix keeps the test
deterministic.
| import { redirectToEmailAccountPath } from "@/utils/account"; | ||
|
|
||
| export default async function OnboardingPage() { | ||
| await redirectToEmailAccountPath("/onboarding"); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify fallback behavior when no default email account is resolved
Ensure redirectToEmailAccountPath("/onboarding") handles edge cases (e.g., user without a selected/default email account, disconnected account) by redirecting to a safe connect/select page.
If missing, I can add a safe fallback (e.g., redirect to a connect/select-account screen with next=/onboarding).
🏁 Script executed:
#!/bin/bash
# Locate the helper definition and inspect its behavior, plus cross-check usages.
set -euo pipefail
echo "Searching for redirectToEmailAccountPath definition with context..."
rg -n --hidden 'export\s+(async\s+)?function\s+redirectToEmailAccountPath|const\s+redirectToEmailAccountPath\s*=\s*' -A 20 -B 2
echo
echo "All usages (to see call patterns and expectations):"
rg -n 'redirectToEmailAccountPath\(' -A 2 -B 2Length of output: 5810
Add safe fallback for missing email account in redirectToEmailAccountPath
Currently, if no email account is found, the helper calls notFound(), yielding a 404. We should instead send the user to a connect/select-account flow and preserve the desired path via a next parameter.
• File: apps/web/utils/account.ts (around line 146)
• Replace:
if (!emailAccount) {
notFound();
}with something like:
if (!emailAccount) {
// send user to select or connect account, then resume at `path`
redirect(`/connect-account?next=${encodeURIComponent(path)}`);
}• Ensure /connect-account (or your actual connect/select page) reads next and redirects back to it after setup.
🤖 Prompt for AI Agents
In apps/web/utils/account.ts around line 146 (called from
apps/web/app/(app)/(redirects)/onboarding/page.tsx), the code currently calls
notFound() when no email account exists; replace that with a safe redirect to a
connect/select-account flow that preserves the original path: if emailAccount is
missing, redirect to a connect page (e.g. /connect-account) including a next
query parameter set to the encoded desired path (use encodeURIComponent(path));
ensure the connect/select-account page reads the next parameter and redirects
back to it after the account is selected/connected; also import and use Next.js
server-side redirect utilities where appropriate.
| const onSubmit = useCallback( | ||
| async (data: CreateRulesOnboardingBody) => { | ||
| // runs in background so we can move on to next step faster | ||
| createRulesOnboardingAction(emailAccountId, data); | ||
| router.push(prefixPath(emailAccountId, "/onboarding?step=4")); | ||
| }, | ||
| [emailAccountId, router], | ||
| ); |
There was a problem hiding this comment.
Add error handling for the background action.
The fire-and-forget pattern on line 67 could lead to silent failures. Consider at least logging errors or showing a non-blocking notification.
Apply this diff to add basic error handling:
const onSubmit = useCallback(
async (data: CreateRulesOnboardingBody) => {
// runs in background so we can move on to next step faster
- createRulesOnboardingAction(emailAccountId, data);
+ createRulesOnboardingAction(emailAccountId, data).catch((error) => {
+ console.error("Failed to create onboarding rules:", error);
+ // Optionally show a non-blocking toast notification
+ });
router.push(prefixPath(emailAccountId, "/onboarding?step=4"));
},
[emailAccountId, router],
);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/app/(app)/[emailAccountId]/onboarding/OnboardingCategories.tsx
around lines 64 to 71, the fire‑and‑forget call to
createRulesOnboardingAction(emailAccountId, data) has no error handling; change
it to handle the returned Promise (without blocking navigation) by appending a
.catch that logs the error and triggers a non‑blocking user notification (e.g.,
console.error/processLogger.error and a toast/snackbar) so failures aren’t
silent; keep router.push(...) immediate, ensure any toast/logger utilities are
imported, and prefer using void createRulesOnboardingAction(...).catch(...) to
make intent explicit.
| // const session = await auth(); | ||
|
|
||
| // if (!session?.user) redirect("/login"); | ||
|
|
||
| // const user = await prisma.user.findUnique({ | ||
| // where: { id: session.user.id }, | ||
| // select: { completedAppOnboardingAt: true, surveyRole: true }, | ||
| // }); | ||
|
|
||
| // if (!user) redirect("/login"); | ||
|
|
||
| // if (!searchParams.force && user.completedAppOnboardingAt) { | ||
| // redirect(env.NEXT_PUBLIC_APP_HOME_PATH); | ||
| // } |
There was a problem hiding this comment.
Remove or implement authentication logic
The authentication and redirect logic is commented out. This should either be implemented or removed entirely for production.
If authentication is required, uncomment and implement:
- // const session = await auth();
-
- // if (!session?.user) redirect("/login");
+ const session = await auth();
+
+ if (!session?.user) redirect("/login");📝 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 session = await auth(); | |
| // if (!session?.user) redirect("/login"); | |
| // const user = await prisma.user.findUnique({ | |
| // where: { id: session.user.id }, | |
| // select: { completedAppOnboardingAt: true, surveyRole: true }, | |
| // }); | |
| // if (!user) redirect("/login"); | |
| // if (!searchParams.force && user.completedAppOnboardingAt) { | |
| // redirect(env.NEXT_PUBLIC_APP_HOME_PATH); | |
| // } | |
| const session = await auth(); | |
| if (!session?.user) redirect("/login"); | |
| // const user = await prisma.user.findUnique({ | |
| // where: { id: session.user.id }, | |
| // select: { completedAppOnboardingAt: true, surveyRole: true }, | |
| // }); | |
| // if (!user) redirect("/login"); | |
| // if (!searchParams.force && user.completedAppOnboardingAt) { | |
| // redirect(env.NEXT_PUBLIC_APP_HOME_PATH); | |
| // } |
🤖 Prompt for AI Agents
In apps/web/app/(app)/[emailAccountId]/onboarding/page.tsx around lines 20 to
33, the authentication and redirect logic is currently commented out; either
restore and implement it or remove it entirely. If auth is required, uncomment
and call the auth helper to get the session, validate session.user, fetch the
user from prisma selecting completedAppOnboardingAt and surveyRole, and perform
redirects to /login or env.NEXT_PUBLIC_APP_HOME_PATH when appropriate
(respecting searchParams.force); if you choose to remove auth, delete the
commented block and ensure downstream code does not rely on session or user
values.
| async getPreviousConversationMessages( | ||
| messageIds: string[], | ||
| ): Promise<ParsedMessage[]> { | ||
| return this.getThreadMessages(messageIds[0]); | ||
| } |
There was a problem hiding this comment.
getPreviousConversationMessages likely uses a messageId where a threadId is required
This calls getThreadMessages(messageIds[0]) but the function expects a thread/conversation ID. Fetch the message to obtain its conversationId/threadId first.
Apply this diff:
- async getPreviousConversationMessages(
- messageIds: string[],
- ): Promise<ParsedMessage[]> {
- return this.getThreadMessages(messageIds[0]);
- }
+ async getPreviousConversationMessages(
+ messageIds: string[],
+ ): Promise<ParsedMessage[]> {
+ if (!messageIds.length) return [];
+ const first = await this.getMessage(messageIds[0]);
+ const threadId = first.threadId;
+ if (!threadId) return [];
+ return this.getThreadMessages(threadId);
+ }📝 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 getPreviousConversationMessages( | |
| messageIds: string[], | |
| ): Promise<ParsedMessage[]> { | |
| return this.getThreadMessages(messageIds[0]); | |
| } | |
| async getPreviousConversationMessages( | |
| messageIds: string[], | |
| ): Promise<ParsedMessage[]> { | |
| if (!messageIds.length) return []; | |
| const first = await this.getMessage(messageIds[0]); | |
| const threadId = first.threadId; | |
| if (!threadId) return []; | |
| return this.getThreadMessages(threadId); | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 275 to 279,
getPreviousConversationMessages currently calls getThreadMessages(messageIds[0])
but that first element is a messageId not a thread/conversationId; change it to
first fetch the message by ID, extract its conversationId (or threadId), and
then call getThreadMessages(conversationId). Also handle the case where the
message fetch returns null or conversationId is missing by returning an empty
array or throwing a clear error, and propagate any API errors appropriately.
| const client = this.client.getClient(); | ||
|
|
||
| // Build the filter query for Microsoft Graph API | ||
| function getFilter() { | ||
| const filters: string[] = []; | ||
|
|
||
| // 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.
Incorrect folder filtering in getThreadsWithQuery (uses names instead of IDs)
parentFolderId must be compared to folder IDs, not string literals like 'inbox'/'archive'. Also, avoid lowercasing labelId (IDs are opaque). Use getFolderIds and inject the actual IDs.
Apply this diff:
- const client = this.client.getClient();
+ const client = this.client.getClient();
+ const folderIds = await getFolderIds(this.client);
// Build the filter query for Microsoft Graph API
function getFilter() {
const filters: string[] = [];
// 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()}'`);
+ // Use labelId as parentFolderId (keep original casing)
+ filters.push(`parentFolderId eq '${query.labelId}'`);
} else if (query?.type === "all") {
- // For "all" type, include both inbox and archive
- filters.push(
- "(parentFolderId eq 'inbox' or parentFolderId eq 'archive')",
- );
+ // For "all" type, include both inbox and archive if available
+ const inboxId = folderIds.inbox;
+ const archiveId = folderIds.archive;
+ if (archiveId) {
+ filters.push(
+ `(parentFolderId eq '${inboxId}' or parentFolderId eq '${archiveId}')`,
+ );
+ } else {
+ filters.push(`parentFolderId eq '${inboxId}'`);
+ }
} else {
- // Default to inbox only
- filters.push("parentFolderId eq 'inbox'");
+ // Default to inbox only
+ filters.push(`parentFolderId eq '${folderIds.inbox}'`);
}📝 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 client = this.client.getClient(); | |
| // Build the filter query for Microsoft Graph API | |
| function getFilter() { | |
| const filters: string[] = []; | |
| // 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'"); | |
| } | |
| const client = this.client.getClient(); | |
| const folderIds = await getFolderIds(this.client); | |
| // Build the filter query for Microsoft Graph API | |
| function getFilter() { | |
| const filters: string[] = []; | |
| // Add folder filter based on type or labelId | |
| if (query?.labelId) { | |
| // Use labelId as parentFolderId (keep original casing) | |
| filters.push(`parentFolderId eq '${query.labelId}'`); | |
| } else if (query?.type === "all") { | |
| // For "all" type, include both inbox and archive if available | |
| const inboxId = folderIds.inbox; | |
| const archiveId = folderIds.archive; | |
| if (archiveId) { | |
| filters.push( | |
| `(parentFolderId eq '${inboxId}' or parentFolderId eq '${archiveId}')`, | |
| ); | |
| } else { | |
| filters.push(`parentFolderId eq '${inboxId}'`); | |
| } | |
| } else { | |
| // Default to inbox only | |
| filters.push(`parentFolderId eq '${folderIds.inbox}'`); | |
| } |
🤖 Prompt for AI Agents
In apps/web/utils/email/microsoft.ts around lines 529 to 547,
getThreadsWithQuery builds filters comparing parentFolderId to string literals
('inbox'/'archive') and lowercases query.labelId; parentFolderId must be
compared to actual folder IDs. Replace those string literals with the real
folder IDs obtained from getFolderIds (e.g., resolve inbox and archive IDs once)
and do not mutate or lowercase query.labelId — use it as the opaque ID; then
push filter expressions that compare parentFolderId eq '<actual-id>' (or
parentFolderId in a constructed OR expression) so the Graph API filters by IDs,
not names. Ensure getFolderIds is awaited before building filters and handle
missing IDs gracefully.
| 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.
Use Buffer instead of atob for base64; atob is not available in Node
atob is a browser API and will crash in Node runtimes. Use Buffer.from(..., 'base64') for decoding.
Apply this diff:
isReplyInThread(message: ParsedMessage): boolean {
try {
- return atob(message.conversationIndex || "").length > 22;
+ const decodedLength = Buffer.from(
+ message.conversationIndex || "",
+ "base64",
+ ).length;
+ return decodedLength > 22;
} catch (error) {
logger.warn("Invalid conversationIndex base64", {
conversationIndex: message.conversationIndex,
error,
});
return false;
}
}📝 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 { | |
| const decodedLength = Buffer.from( | |
| message.conversationIndex || "", | |
| "base64", | |
| ).length; | |
| return decodedLength > 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 code uses atob
to decode a base64 conversationIndex which fails in Node; replace the atob call
with Buffer.from(message.conversationIndex || "", "base64") and check the
resulting Buffer length (e.g., Buffer.from(...).length > 22) instead of string
length, preserving the existing try/catch and logger.warn on error and returning
false on failure.
| import type { ParsedMessage } from "@/utils/types"; | ||
| import type { InboxZeroLabel } from "@/utils/label"; | ||
| import type { ThreadsQuery } from "@/app/api/threads/validation"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid cross-layer coupling to app route types
Importing ThreadsQuery from an API route ties core types to a route module. Prefer moving the Zod schema/type to a shared utils module or define a lightweight provider-facing type here.
Minimal alternative:
// In a shared module (e.g., "@/utils/email/queries")
export type ThreadsQuery = {
label?: string;
q?: string;
before?: string;
after?: string;
pageToken?: string;
};Then update this file to import from the shared path.
Also applies to: 123-131
🏁 Script executed:
#!/bin/bash
# Locate the definition of ThreadsQuery
rg "export (type|const) ThreadsQuery" -n
# Show import usage in utils/email/types.ts
rg -n "ThreadsQuery" apps/web/utils/email/types.ts
# If found, print the definition context
VALIDATION_FILE=$(rg -l "export (type|const) ThreadsQuery" -n)
if [ -n "$VALIDATION_FILE" ]; then
echo "Definition found in: $VALIDATION_FILE"
echo "--- First 50 lines of $VALIDATION_FILE ---"
head -n 50 "$VALIDATION_FILE"
else
echo "No ThreadsQuery definition found in the repo."
fiLength of output: 1117
Refactor cross‐layer dependency for ThreadsQuery
Importing ThreadsQuery directly from apps/web/app/api/threads/validation.ts couples your email utilities to an application‐layer route. Extract the Zod schema and its inferred type into a shared module and have both the API route and your utils import from it:
• Create apps/web/utils/email/queries.ts (or a dedicated shared folder) and move:
export const threadsQuery = z.object({
fromEmail: z.string().nullish(),
limit: z.coerce.number().max(100).nullish(),
type: z.string().nullish(),
q: z.string().nullish(),
nextPageToken: z.string().nullish(),
labelId: z.string().nullish(),
});
export type ThreadsQuery = z.infer<typeof threadsQuery>;• In apps/web/app/api/threads/validation.ts:
- import { z } from "zod";
+ import { threadsQuery } from "@/utils/email/queries";
export const threadsQuery = threadsQuery;
export type ThreadsQuery = z.infer<typeof threadsQuery>;• In apps/web/utils/email/types.ts:
- import type { ThreadsQuery } from "@/app/api/threads/validation";
+ import type { ThreadsQuery } from "@/utils/email/queries";This keeps your core types in a shared layer and your API routes focused on validation.
🤖 Prompt for AI Agents
In apps/web/utils/email/types.ts around lines 1 to 4, the file imports
ThreadsQuery from the application route which couples utils to the app layer;
extract the Zod schema and inferred type into a shared module (e.g.,
apps/web/utils/email/queries.ts), move the threadsQuery z.object and export type
ThreadsQuery = z.infer<typeof threadsQuery> there, then update both
apps/web/app/api/threads/validation.ts and apps/web/utils/email/types.ts to
import threadsQuery/ThreadsQuery from the new shared module so utils and the API
route depend on the shared types rather than each other.
| typeof item.content.content === "string" ? ( | ||
| item.content.content | ||
| .split("\n") | ||
| .filter((line) => line.trim()) | ||
| .map((line, idx) => ( | ||
| <Text | ||
| key={idx} | ||
| className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]" | ||
| > | ||
| {line} | ||
| </Text> | ||
| )) | ||
| ) : ( | ||
| item.content.content.toString() | ||
| ) | ||
| ) : null} |
There was a problem hiding this comment.
Avoid index keys; wrap fallback in Text for consistency
- Replace index keys with a stable key to comply with guidelines.
- For non-string unstructured content, render inside a Text block to keep markup consistent across email clients.
- item.content.content
- .split("\n")
- .filter((line) => line.trim())
- .map((line, idx) => (
- <Text
- key={idx}
- className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]"
- >
- {line}
- </Text>
- ))
+ item.content.content
+ .split("\n")
+ .filter((line) => line.trim())
+ .map((line) => (
+ <Text
+ key={`${line}-${line.length}`}
+ className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]"
+ >
+ {line}
+ </Text>
+ ))
) : (
- item.content.content.toString()
+ <Text className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]">
+ {item.content.content.toString()}
+ </Text>
)If duplicate lines are possible, consider hashing the line for a collision-resistant key; I can add a tiny helper if preferred.
📝 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.
| typeof item.content.content === "string" ? ( | |
| item.content.content | |
| .split("\n") | |
| .filter((line) => line.trim()) | |
| .map((line, idx) => ( | |
| <Text | |
| key={idx} | |
| className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]" | |
| > | |
| {line} | |
| </Text> | |
| )) | |
| ) : ( | |
| item.content.content.toString() | |
| ) | |
| ) : null} | |
| typeof item.content.content === "string" ? ( | |
| item.content.content | |
| .split("\n") | |
| .filter((line) => line.trim()) | |
| .map((line) => ( | |
| <Text | |
| key={`${line}-${line.length}`} | |
| className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]" | |
| > | |
| {line} | |
| </Text> | |
| )) | |
| ) : ( | |
| <Text className="text-[14px] text-gray-500 mt-[2px] m-0 leading-[21px]"> | |
| {item.content.content.toString()} | |
| </Text> | |
| ) | |
| ) : null} |
🤖 Prompt for AI Agents
In packages/resend/emails/digest.tsx around lines 329 to 344, the map uses the
array index as the React key and the non-string fallback is rendered without a
Text wrapper; replace the index key with a stable one (e.g., the line string or
a hashed value of the line to avoid collisions if duplicates are possible) and
wrap the non-string fallback (item.content.content.toString()) inside the same
Text component used for lines so markup is consistent across email clients; if
duplicate lines may occur, add or call a small hash helper to produce
collision-resistant keys.
2 participants
Summary by CodeRabbit