Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a series of updates across the codebase. It removes an unused import in the summarization controller and adds new asynchronous functions for generating AI nudges and replies. Several import paths have been restructured to consolidate utility functions, with significant updates to date handling using the new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Email Component
participant GN as generateNudgeAction
participant Auth as auth
participant UserDB as getAiUserByEmail
participant Chat as chatCompletion
UI->>GN: Request nudge generation with message data
GN->>Auth: Authenticate user
Auth-->>GN: Return user credentials
GN->>UserDB: Fetch AI user details by email
UserDB-->>GN: Return user information
GN->>Chat: Call chatCompletion with prompt & data
Chat-->>GN: Return generated nudge text
GN-->>UI: Deliver nudge response
sequenceDiagram
participant UI as Email Component
participant GR as generateReplyAction
participant Auth as auth
participant UserDB as getAiUserByEmail
participant DB as Reply Database
participant Chat as chatCompletion
UI->>GR: Request reply generation with thread messages
GR->>Auth: Check user authentication
Auth-->>GR: Validated credentials
GR->>UserDB: Retrieve AI user details
UserDB-->>GR: Return user data
GR->>DB: Query for existing reply
alt Reply Found
DB-->>GR: Return existing reply
else No Reply Found
GR->>Chat: Request reply generation with formatted messages
Chat-->>GR: Return new reply text
GR->>DB: Save generated reply
end
GR-->>UI: Return reply text
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
apps/web/app/api/ai/reply/nudge/route.ts (2)
8-19: Consider validating the date field.
Currently, the schema declares 'date' as just a string. For better robustness, you might want to ensure it's a valid date format or use zod’s built-in date validation (if available in your version) to avoid potential runtime errors when parsing an invalid date string.
41-41: Resolve the TODO for parsing HTML content.
The code references parsing HTML content but is not implemented. This might cause issues if only textHtml is populated.Would you like me to propose a snippet to extract the text from HTML or handle the rendered HTML directly?
apps/web/utils/ai/reply/generate-reply.ts (1)
27-37: Be aware of potential prompt size limits.
Concatenating multiple emails into a single prompt can exceed token limits. Consider truncating or summarizing older parts of the conversation more aggressively if messages become huge.apps/web/utils/ai/reply/generate-nudge.ts (3)
6-6: Uncomment the logger setup.The logger is commented out but could be useful for debugging and monitoring in production.
-// const logger = createScopedLogger("generate-nudge"); +const logger = createScopedLogger("generate-nudge");
8-20: Consider adding input validation.The function accepts messages without validating their content length or format. Consider adding validation to ensure the input data meets your requirements before processing.
export async function aiGenerateNudge({ messages, user, }: { messages: { from: string; to: string; subject: string; content: string; date: Date; }[]; user: UserEmailWithAI; +}) { + if (!messages.length) { + throw new Error("At least one message is required"); + } + + if (!messages.every(msg => msg.content.trim())) { + throw new Error("All messages must have non-empty content"); + } }) {
21-25: Consider parameterizing the system prompt.The system prompt is hardcoded. Consider making it configurable or moving it to a constants file for better maintainability.
+const NUDGE_SYSTEM_PROMPT = `You are an AI assistant helping to write a follow-up email to nudge someone who hasn't responded. +Write a polite and professional email that follows up on the previous conversation. +Keep it concise and friendly. Don't be pushy. +Use context from the previous emails to make it relevant. +Don't mention that you're an AI.`; export async function aiGenerateNudge({ messages, user, }: { messages: { from: string; to: string; subject: string; content: string; date: Date; }[]; user: UserEmailWithAI; }) { - const system = `You are an AI assistant helping to write a follow-up email to nudge someone who hasn't responded. -Write a polite and professional email that follows up on the previous conversation. -Keep it concise and friendly. Don't be pushy. -Use context from the previous emails to make it relevant. -Don't mention that you're an AI.`; + const system = NUDGE_SYSTEM_PROMPT;apps/web/app/api/ai/reply/route.ts (2)
8-19: Consider adding more validation constraints.The message schema could benefit from additional validation:
- Email format validation for
fromandto- Maximum length constraints for
subjectand content fields- Date format validation
const messageSchema = z .object({ - from: z.string(), - to: z.string(), + from: z.string().email(), + to: z.string().email(), - subject: z.string(), + subject: z.string().max(200), - textPlain: z.string().optional(), - textHtml: z.string().optional(), + textPlain: z.string().max(50000).optional(), + textHtml: z.string().max(100000).optional(), - date: z.string(), + date: z.string().refine((val) => !isNaN(Date.parse(val)), { + message: "Invalid date format", + }), })
25-48: Consider rate limiting and response size limits.The API endpoint should implement rate limiting and set maximum response size limits to prevent abuse.
Consider using a middleware for rate limiting:
import rateLimit from 'express-rate-limit' const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }); export const POST = withError( limiter(async (request: Request) => { // ... existing code ... }) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/app/api/ai/reply/nudge/route.ts(1 hunks)apps/web/app/api/ai/reply/route.ts(1 hunks)apps/web/app/api/ai/summarise/controller.ts(0 hunks)apps/web/utils/ai/reply/generate-nudge.ts(1 hunks)apps/web/utils/ai/reply/generate-reply.ts(1 hunks)apps/web/utils/user/get.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/api/ai/summarise/controller.ts
🔇 Additional comments (5)
apps/web/app/api/ai/reply/nudge/route.ts (1)
47-47: Confirm the streaming response aligns with Next.js conventions.
Returning the result directly as a stream is likely correct for your streaming use case; just verify that Next.js (or your framework) supports streaming responses in all runtime environments (e.g., Edge, serverful environments).apps/web/utils/user/get.ts (1)
1-15: Validate uniqueness of the user email for consistent findUnique usage.
If multiple users share the same email (which is a rare edge case), findUnique could fail. Confirm that there is a unique index on the 'email' column to guarantee correct behavior.apps/web/utils/ai/reply/generate-reply.ts (2)
21-25: Check for brand consistency and legal disclaimers in AI-generated emails.
Your system prompts the AI to refrain from mentioning it’s an AI. Ensure that this approach aligns with business/legal guidelines.
39-47: Verify usage label and streaming completion flow.
Labeling usage as "Reply" is a good start; confirm in your analytics or logs that this usage label is properly tracked. Also ensure the streaming flow handles partial or truncated responses gracefully if tokens run out.apps/web/app/api/ai/reply/route.ts (1)
41-42: Address the TODO comment about HTML parsing.The code currently uses raw HTML content as a fallback. Consider implementing proper HTML parsing to extract clean text content.
Would you like me to help implement HTML parsing using a library like
html-to-text?
| export const POST = withError(async (request: Request) => { | ||
| const session = await auth(); | ||
| if (!session?.user.email) | ||
| return NextResponse.json({ error: "Not authenticated" }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return proper HTTP status codes for errors.
Returning JSON with an “error” field and defaulting to HTTP 200 can confuse clients expecting 401 for unauthorized or 404 for user-not-found. Consider adding status codes to give a precise indication of the failure reason.
| const response = await chatCompletionStream({ | ||
| userAi: user, | ||
| system, | ||
| prompt, | ||
| userEmail: user.email, | ||
| usageLabel: "Reply", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for chatCompletionStream.
The function should handle potential errors from the chatCompletionStream call.
+ try {
const response = await chatCompletionStream({
userAi: user,
system,
prompt,
userEmail: user.email,
usageLabel: "Reply",
});
+ return response.toTextStreamResponse();
+ } catch (error) {
+ logger.error("Error generating nudge", { error });
+ throw new Error("Failed to generate nudge email");
+ }
- return response.toTextStreamResponse();📝 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 response = await chatCompletionStream({ | |
| userAi: user, | |
| system, | |
| prompt, | |
| userEmail: user.email, | |
| usageLabel: "Reply", | |
| }); | |
| try { | |
| const response = await chatCompletionStream({ | |
| userAi: user, | |
| system, | |
| prompt, | |
| userEmail: user.email, | |
| usageLabel: "Reply", | |
| }); | |
| return response.toTextStreamResponse(); | |
| } catch (error) { | |
| logger.error("Error generating nudge", { error }); | |
| throw new Error("Failed to generate nudge email"); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/app/api/user/stats/tinybird/load/load-emails.ts (1)
182-189: Consider adding fallback for timestamp.The current implementation skips emails without a timestamp. Consider adding a fallback mechanism or logging more context for debugging.
if (!tinybirdEmail.timestamp) { logger.error("No timestamp for email", { ownerEmail: tinybirdEmail.ownerEmail, gmailMessageId: tinybirdEmail.gmailMessageId, date: m.internalDate, + headers: m.headers, + fallbackTimestamp: +new Date() }); - return; + tinybirdEmail.timestamp = +new Date(); // Use current timestamp as fallback }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web/app/(app)/cold-email-blocker/TestRules.tsx(1 hunks)apps/web/app/(app)/simple/SimpleList.tsx(0 hunks)apps/web/app/api/ai/categorize/validation.ts(1 hunks)apps/web/app/api/google/webhook/process-history-item.ts(2 hunks)apps/web/app/api/user/stats/tinybird/load/load-emails.ts(3 hunks)apps/web/components/email-list/EmailList.tsx(1 hunks)apps/web/utils/actions/categorize-email.ts(2 hunks)apps/web/utils/actions/cold-email.ts(1 hunks)apps/web/utils/assistant/process-assistant-email.ts(2 hunks)apps/web/utils/cold-email/is-cold-email.ts(3 hunks)apps/web/utils/get-email-from-message.ts(2 hunks)apps/web/utils/gmail/message.ts(2 hunks)apps/web/utils/types.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/(app)/simple/SimpleList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/utils/cold-email/is-cold-email.ts
🔇 Additional comments (10)
apps/web/utils/get-email-from-message.ts (1)
3-3: LGTM! Good improvement in date handling.The changes standardize date handling by using Gmail's internal date instead of the email header date, which is more reliable.
Also applies to: 15-15
apps/web/app/api/ai/categorize/validation.ts (1)
20-20: LGTM! Consistent with date handling changes.The schema now correctly uses
internalDatefrom Gmail's API instead of the email header date.apps/web/utils/actions/categorize-email.ts (1)
16-16: LGTM! Good improvements in code structure and type safety.The changes:
- Use object parameter pattern for better readability
- Convert string date to Date object for type safety
Also applies to: 43-47
apps/web/utils/types.ts (1)
96-96: LGTM! Good documentation.The comment clearly explains the date handling strategy, distinguishing between email header date and Gmail's internal date.
apps/web/utils/actions/cold-email.ts (1)
125-125: LGTM! Consistent date handling.The change ensures type safety and consistent date handling by explicitly converting the date string to a Date object.
apps/web/utils/gmail/message.ts (1)
104-104: LGTM! Enhanced type safety for date parameters.The function signatures now correctly specify Date objects instead of strings, improving type safety and consistency.
Also applies to: 136-136
apps/web/app/api/user/stats/tinybird/load/load-emails.ts (1)
173-173: LGTM! Improved date handling with dedicated utility.Using internalDateToDate provides consistent date handling across the codebase.
Also applies to: 186-186
apps/web/app/api/google/webhook/process-history-item.ts (1)
142-142: LGTM! Consistent date handling in cold email blocker.The change aligns with the codebase-wide standardization of date handling using internalDateToDate.
apps/web/app/(app)/cold-email-blocker/TestRules.tsx (1)
171-171: LGTM! Consistent date handling.The change aligns with the codebase-wide shift to using
internalDatefor date handling.apps/web/utils/assistant/process-assistant-email.ts (1)
13-13: LGTM! Standardized date handling.The changes improve consistency by using the centralized
internalDateToDateutility for date conversions.Also applies to: 173-175, 178-180
| cc?: string; | ||
| subject: string; | ||
| content: string; | ||
| date?: Date; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if date should be optional.
The date property is marked as optional (date?: Date). Given that internalDate is required in the categorization schema, should this be required here as well?
Let's check how this type is used:
🏁 Script executed:
#!/bin/bash
# Search for usages of EmailForLLM type and date property
rg -A 5 "EmailForLLM" && rg "date.*Date" --type tsLength of output: 20626
Action: Remove the optional modifier on the “date” property.
After reviewing the usage across the codebase and seeing that functions like getEmailForLLM always provide a date value (whether as a string via ISO format or as a parsed Date), it’s clear that consumers are built with the assumption that a date is always present. In the type definition (in apps/web/utils/types.ts), the property appears twice—once as a required string and again as an optional Date—which is both redundant and potentially error‑prone given that the categorization schema requires an internal date. The fix is to remove the optional modifier (or refactor to use differently named properties if both representations are needed).
- Location:
apps/web/utils/types.ts(definition ofEmailForLLM) - Issue: Duplicate declaration of
dateexists (a required string and an optional Date) causing inconsistency with the categorization schema's expectation that a date always be provided.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/utils/ai/reply/generate-nudge.ts (1)
8-55: Consider extracting common code into a shared utility.The
aiGenerateNudgefunction shares significant code withaiGenerateReply. Consider extracting the common functionality into a shared utility to improve maintainability.// shared-email-generator.ts interface EmailGeneratorConfig { messages: EmailMessage[]; user: UserEmailWithAI; systemPrompt: string; userPrompt: string; onFinish?: (completion: string) => Promise<void>; } async function generateEmail({ messages, user, systemPrompt, userPrompt, onFinish, }: EmailGeneratorConfig) { // Common validation, formatting, and generation logic } // generate-nudge.ts export async function aiGenerateNudge({ messages, user, onFinish }) { return generateEmail({ messages, user, systemPrompt: `You are an AI assistant...`, userPrompt: `Please write a follow-up email...`, onFinish, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/app/api/ai/reply/nudge/route.ts(1 hunks)apps/web/app/api/ai/reply/route.ts(1 hunks)apps/web/providers/SWRProvider.tsx(1 hunks)apps/web/utils/ai/reply/generate-nudge.ts(1 hunks)apps/web/utils/ai/reply/generate-reply.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/api/ai/reply/route.ts
🔇 Additional comments (3)
apps/web/utils/ai/reply/generate-reply.ts (2)
8-20: Add input validation for messages array.The function should validate that the messages array is not empty and properly ordered by date to ensure correct context flow.
if (!messages.length) { throw new Error("No messages provided"); } // Verify messages are ordered by date const isOrdered = messages.every((msg, i) => i === 0 || msg.date >= messages[i-1].date ); if (!isOrdered) { throw new Error("Messages must be ordered by date"); }
39-49: Add error handling for chatCompletionStream.The function should handle potential errors from the chatCompletionStream call.
+ try { const response = await chatCompletionStream({ userAi: user, system, prompt, userEmail: user.email, usageLabel: "Reply", onFinish: async (completion) => { logger.trace("Output", { completion }); }, }); + return response.toDataStreamResponse(); + } catch (error) { + logger.error("Error generating reply", { error }); + throw new Error("Failed to generate reply email"); + } - return response.toDataStreamResponse();apps/web/app/api/ai/reply/nudge/route.ts (1)
29-33: Return proper HTTP status codes for errors.Return appropriate HTTP status codes instead of defaulting to 200.
- return NextResponse.json({ error: "Not authenticated" }); + return NextResponse.json( + { error: "Not authenticated" }, + { status: 401 } + );
| const user = await getAiUserByEmail({ email: session.user.email }); | ||
|
|
||
| if (!user) return NextResponse.json({ error: "User not found" }); | ||
|
|
There was a problem hiding this comment.
Add proper error handling for database operations.
The getAiUserByEmail call should be wrapped in a try-catch block to handle potential database errors.
+ try {
const user = await getAiUserByEmail({ email: session.user.email });
if (!user) return NextResponse.json(
{ error: "User not found" },
{ status: 404 }
);
+ } catch (error) {
+ logger.error("Database error", { error });
+ return NextResponse.json(
+ { error: "Internal server error" },
+ { status: 500 }
+ );
+ }📝 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 user = await getAiUserByEmail({ email: session.user.email }); | |
| if (!user) return NextResponse.json({ error: "User not found" }); | |
| try { | |
| const user = await getAiUserByEmail({ email: session.user.email }); | |
| if (!user) | |
| return NextResponse.json( | |
| { error: "User not found" }, | |
| { status: 404 } | |
| ); | |
| } catch (error) { | |
| logger.error("Database error", { error }); | |
| return NextResponse.json( | |
| { error: "Internal server error" }, | |
| { status: 500 } | |
| ); | |
| } |
elie222
force-pushed
the
generate-reply
branch
from
41e6a1e to
061cad9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/utils/ai/reply/generate-nudge.ts (1)
43-50: Add error handling for chatCompletion call.The function should handle potential errors from the chatCompletion call.
- const response = await chatCompletion({ - userAi: user, - system, - prompt, - userEmail: user.email, - usageLabel: "Reply", - }); + try { + const response = await chatCompletion({ + userAi: user, + system, + prompt, + userEmail: user.email, + usageLabel: "Reply", + }); + return response.text; + } catch (error) { + logger.error("Error generating nudge", { error }); + throw new Error("Failed to generate nudge email"); + } - return response.text;
🧹 Nitpick comments (4)
apps/web/components/email-list/EmailThread.tsx (1)
315-318: Potential content overwriting
Usingcompletion || replyingToEmail.draftHtmlmight overwrite the user's typed draft if a new AI completion is available. Confirm that you want to discard manual changes if there's a valid completion. Alternatively, consider merging both inputs or prompting user confirmation.apps/web/utils/actions/generate-reply.validation.ts (1)
1-22: Strong validation approach using Zod
This schema ensures that eithertextPlainortextHtmlis present, which is good. Consider also enforcing minimal length or sanitizing inputs (e.g., removing harmful HTML tags) if there's a risk of injection or excessively large payloads.apps/web/utils/ai/reply/generate-reply.ts (1)
1-53: Ensure robust error handling
Consider wrappingchatCompletionin a try/catch block to gracefully handle network or AI service errors. Also, confirm that any truncated content beyond 3000 characters doesn't adversely affect reply quality. If necessary, split long emails into multiple chunks for more comprehensive context.apps/web/utils/ai/reply/generate-nudge.ts (1)
22-27: Enhance system prompt with specific guidelines.The system prompt could be more specific about:
- Maximum length of the follow-up email
- Tone variations based on the number of previous follow-ups
- Guidelines for maintaining thread context
const system = `You are an AI assistant helping to write a follow-up email to nudge someone who hasn't responded. Write a polite and professional email that follows up on the previous conversation. -Keep it concise and friendly. Don't be pushy. +Keep it concise (maximum 4-5 sentences) and friendly. Adjust tone based on the number of previous follow-ups. Use context from the previous emails to make it relevant. +Reference specific points or dates from the previous conversation when appropriate. Don't mention that you're an AI. Don't reply with a Subject. Only reply with the body of the email.`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/api/ai/reply/nudge/route.ts(1 hunks)apps/web/components/email-list/EmailThread.tsx(6 hunks)apps/web/utils/actions/generate-reply.ts(1 hunks)apps/web/utils/actions/generate-reply.validation.ts(1 hunks)apps/web/utils/ai/reply/generate-nudge.ts(1 hunks)apps/web/utils/ai/reply/generate-reply.ts(1 hunks)apps/web/utils/auth.ts(1 hunks)apps/web/utils/llms/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/utils/auth.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/api/ai/reply/nudge/route.ts
🔇 Additional comments (4)
apps/web/components/email-list/EmailThread.tsx (3)
32-33: Use named imports responsibly
Your import usage from"ai/react"and the typedGenerateReplyBodyare consistent. Ensure your environment is configured to properly handle AI streaming features, and that theGenerateReplyBodytype definition matches the structure required by the backend.
92-119: Confirm plain text drafting scenario
You conditionally setgenerateNudgetodefaultShowReply && !draftReply?.textHtml, which might unintentionally trigger a nudge if there's only plain text in the draft. If you expect drafts with no HTML content but plain text, consider also checkingdraftReply?.textPlain.
134-134: New 'generateNudge' prop introduced
Extending theEmailMessageprops withgenerateNudgeis a clear approach. Ensure that all call sites supply a boolean value or omit it, to prevent undefined behavior when referencing this prop.Also applies to: 144-144
apps/web/utils/ai/reply/generate-nudge.ts (1)
8-21: Consider adding input validation for messages array.The function accepts an array of messages but doesn't validate the minimum number of messages required for context.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if there are any existing validation checks for minimum messages rg -A 3 "messages.length" apps/web/utils/ai/replyLength of output: 49
Missing Input Validation for Messages Array
The investigation confirms that there are no checks in place to validate the length of the
messagesarray before proceeding. This means that if the array does not meet the minimum required count for context, the function might operate on unexpected input or even fail.
- Issue Identified: No validation to ensure the
messagesarray has sufficient entries.- Recommendation: Add a guard clause to check that the number of messages meets the minimum requirement. If not, either throw an error or handle the scenario gracefully.
| const body: GenerateReplyBody = { | ||
| messages: [ | ||
| { | ||
| id: message.id, | ||
| textHtml: message.textHtml, | ||
| textPlain: message.textPlain, | ||
| date: message.headers.date, | ||
| from: message.headers.from, | ||
| to: message.headers.to, | ||
| subject: message.headers.subject, | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| const { completion, complete, error, isLoading } = useCompletion({ | ||
| api: "/api/ai/reply/nudge", | ||
| body, | ||
| }); | ||
| console.log("🚀 ~ completion:", completion); | ||
|
|
||
| if (error) { | ||
| console.error("There was an error generating the nudge", error); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| if (generateNudge) { | ||
| // we send the data via the body instead | ||
| complete(""); | ||
| } | ||
| }, [complete, generateNudge]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent repeated AI calls & remove console logs
- Triggering
complete("")in auseEffectwhenevergenerateNudgeis true can cause repeated calls if the prop toggles multiple times. Consider adding a check or controlling repeated invocations if needed. - Remove the
console.log("🚀 ~ completion:", completion);statement or replace it with a formal logger before production.
| export async function chatCompletion({ | ||
| userAi, | ||
| prompt, | ||
| system, | ||
| userEmail, | ||
| usageLabel, | ||
| }: { | ||
| userAi: UserAIFields; | ||
| prompt: string; | ||
| system?: string; | ||
| userEmail: string; | ||
| usageLabel: string; | ||
| }) { | ||
| try { | ||
| const { provider, model, llmModel } = getModel(userAi); | ||
|
|
||
| const result = await generateText({ | ||
| model: llmModel, | ||
| prompt, | ||
| system, | ||
| experimental_telemetry: { isEnabled: true }, | ||
| }); | ||
|
|
||
| if (result.usage) { | ||
| await saveAiUsage({ | ||
| email: userEmail, | ||
| usage: result.usage, | ||
| provider, | ||
| model, | ||
| label: usageLabel, | ||
| }); | ||
| } | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| await handleError(error, userEmail); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout and retry mechanism for transient errors.
The chatCompletion function should handle timeouts and implement retries for transient errors, similar to the existing withRetry utility.
export async function chatCompletion({
userAi,
prompt,
system,
userEmail,
usageLabel,
+ timeout = 30000,
+ maxRetries = 3,
}: {
userAi: UserAIFields;
prompt: string;
system?: string;
userEmail: string;
usageLabel: string;
+ timeout?: number;
+ maxRetries?: number;
}) {
- try {
+ return withRetry(
+ async () => {
+ try {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), timeout);
+
+ try {
const { provider, model, llmModel } = getModel(userAi);
const result = await generateText({
model: llmModel,
prompt,
system,
experimental_telemetry: { isEnabled: true },
+ signal: controller.signal,
});
if (result.usage) {
await saveAiUsage({
email: userEmail,
usage: result.usage,
provider,
model,
label: usageLabel,
});
}
return result;
+ } finally {
+ clearTimeout(timeoutId);
+ }
} catch (error) {
await handleError(error, userEmail);
throw error;
}
- }
+ },
+ {
+ retryIf: (error) =>
+ error instanceof APICallError || error instanceof RetryError,
+ maxRetries,
+ delayMs: 1000,
+ },
+ );
}📝 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.
| export async function chatCompletion({ | |
| userAi, | |
| prompt, | |
| system, | |
| userEmail, | |
| usageLabel, | |
| }: { | |
| userAi: UserAIFields; | |
| prompt: string; | |
| system?: string; | |
| userEmail: string; | |
| usageLabel: string; | |
| }) { | |
| try { | |
| const { provider, model, llmModel } = getModel(userAi); | |
| const result = await generateText({ | |
| model: llmModel, | |
| prompt, | |
| system, | |
| experimental_telemetry: { isEnabled: true }, | |
| }); | |
| if (result.usage) { | |
| await saveAiUsage({ | |
| email: userEmail, | |
| usage: result.usage, | |
| provider, | |
| model, | |
| label: usageLabel, | |
| }); | |
| } | |
| return result; | |
| } catch (error) { | |
| await handleError(error, userEmail); | |
| throw error; | |
| } | |
| } | |
| export async function chatCompletion({ | |
| userAi, | |
| prompt, | |
| system, | |
| userEmail, | |
| usageLabel, | |
| timeout = 30000, | |
| maxRetries = 3, | |
| }: { | |
| userAi: UserAIFields; | |
| prompt: string; | |
| system?: string; | |
| userEmail: string; | |
| usageLabel: string; | |
| timeout?: number; | |
| maxRetries?: number; | |
| }) { | |
| return withRetry( | |
| async () => { | |
| try { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), timeout); | |
| try { | |
| const { provider, model, llmModel } = getModel(userAi); | |
| const result = await generateText({ | |
| model: llmModel, | |
| prompt, | |
| system, | |
| experimental_telemetry: { isEnabled: true }, | |
| signal: controller.signal, | |
| }); | |
| if (result.usage) { | |
| await saveAiUsage({ | |
| email: userEmail, | |
| usage: result.usage, | |
| provider, | |
| model, | |
| label: usageLabel, | |
| }); | |
| } | |
| return result; | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| } catch (error) { | |
| await handleError(error, userEmail); | |
| throw error; | |
| } | |
| }, | |
| { | |
| retryIf: (error) => | |
| error instanceof APICallError || error instanceof RetryError, | |
| maxRetries, | |
| delayMs: 1000, | |
| }, | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/components/email-list/EmailThread.tsx(5 hunks)apps/web/utils/actions/generate-reply.ts(1 hunks)
🔇 Additional comments (1)
apps/web/components/email-list/EmailThread.tsx (1)
91-118: LGTM!The
generateNudgeprop is correctly passed to theEmailMessagecomponent and is properly used to conditionally trigger nudge generation.
| export const generateNudgeAction = withActionInstrumentation( | ||
| "generateNudge", | ||
| async (unsafeData: GenerateReplySchema) => { | ||
| const session = await auth(); | ||
| if (!session?.user.email) return { error: "Not authenticated" }; | ||
|
|
||
| const user = await getAiUserByEmail({ email: session.user.email }); | ||
|
|
||
| if (!user) return { error: "User not found" }; | ||
|
|
||
| const { data, error } = generateReplySchema.safeParse(unsafeData); | ||
| if (error) return { error: error.message }; | ||
|
|
||
| const lastMessage = data.messages.at(-1); | ||
|
|
||
| if (!lastMessage) return { error: "No message provided" }; | ||
|
|
||
| const messages = data.messages.map((msg) => ({ | ||
| ...msg, | ||
| date: new Date(msg.date), | ||
| content: emailToContent({ | ||
| textPlain: msg.textPlain, | ||
| textHtml: msg.textHtml, | ||
| snippet: "", | ||
| }), | ||
| })); | ||
|
|
||
| const text = await aiGenerateNudge({ messages, user }); | ||
|
|
||
| return { text }; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code into a shared utility function.
Both actions share identical authentication, validation, and message processing logic. Consider extracting the common code into a shared utility function to improve maintainability.
Apply this diff to reduce code duplication:
+async function processEmailRequest({
+ actionType,
+ unsafeData,
+ generator,
+}: {
+ actionType: "nudge" | "reply";
+ unsafeData: GenerateReplySchema;
+ generator: (args: { messages: any[]; user: any }) => Promise<string>;
+}) {
+ const session = await auth();
+ if (!session?.user.email) return { error: "Not authenticated" };
+
+ const user = await getAiUserByEmail({ email: session.user.email });
+ if (!user) return { error: "User not found" };
+
+ const { data, error } = generateReplySchema.safeParse(unsafeData);
+ if (error) return { error: error.message };
+
+ const lastMessage = data.messages.at(-1);
+ if (!lastMessage) return { error: "No message provided" };
+
+ const messages = data.messages.map((msg) => ({
+ ...msg,
+ date: new Date(msg.date),
+ content: emailToContent({
+ textPlain: msg.textPlain,
+ textHtml: msg.textHtml,
+ snippet: "",
+ }),
+ }));
+
+ return { user, messages, lastMessage };
+}
+
export const generateNudgeAction = withActionInstrumentation(
"generateNudge",
- async (unsafeData: GenerateReplySchema) => {
- const session = await auth();
- if (!session?.user.email) return { error: "Not authenticated" };
-
- const user = await getAiUserByEmail({ email: session.user.email });
- if (!user) return { error: "User not found" };
-
- const { data, error } = generateReplySchema.safeParse(unsafeData);
- if (error) return { error: error.message };
-
- const lastMessage = data.messages.at(-1);
- if (!lastMessage) return { error: "No message provided" };
-
- const messages = data.messages.map((msg) => ({
- ...msg,
- date: new Date(msg.date),
- content: emailToContent({
- textPlain: msg.textPlain,
- textHtml: msg.textHtml,
- snippet: "",
- }),
- }));
-
- const text = await aiGenerateNudge({ messages, user });
- return { text };
+ async (unsafeData: GenerateReplySchema) => {
+ const result = await processEmailRequest({
+ actionType: "nudge",
+ unsafeData,
+ generator: aiGenerateNudge,
+ });
+ if ("error" in result) return result;
+
+ const { user, messages } = result;
+ const text = await aiGenerateNudge({ messages, user });
+ return { text };
},
);
export const generateReplyAction = withActionInstrumentation(
"generateReply",
- async (unsafeData: GenerateReplySchema) => {
- const session = await auth();
- if (!session?.user.email) return { error: "Not authenticated" };
-
- const user = await getAiUserByEmail({ email: session.user.email });
- if (!user) return { error: "User not found" };
-
- const { data, error } = generateReplySchema.safeParse(unsafeData);
- if (error) return { error: error.message };
-
- const lastMessage = data.messages.at(-1);
- if (!lastMessage) return { error: "No message provided" };
-
- const reply = await getReply({
- userId: user.id,
- messageId: lastMessage.id,
- });
-
- if (reply) return { text: reply };
-
- const messages = data.messages.map((msg) => ({
- ...msg,
- date: new Date(msg.date),
- content: emailToContent({
- textPlain: msg.textPlain,
- textHtml: msg.textHtml,
- snippet: "",
- }),
- }));
-
- const text = await aiGenerateReply({ messages, user });
-
- await saveReply({
- userId: user.id,
- messageId: lastMessage.id,
- reply: text,
- });
-
- return { text };
+ async (unsafeData: GenerateReplySchema) => {
+ const result = await processEmailRequest({
+ actionType: "reply",
+ unsafeData,
+ generator: aiGenerateReply,
+ });
+ if ("error" in result) return result;
+
+ const { user, messages, lastMessage } = result;
+
+ const existingReply = await getReply({
+ userId: user.id,
+ messageId: lastMessage.id,
+ });
+ if (existingReply) return { text: existingReply };
+
+ const text = await aiGenerateReply({ messages, user });
+ await saveReply({
+ userId: user.id,
+ messageId: lastMessage.id,
+ reply: text,
+ });
+
+ return { text };
},
);Also applies to: 48-92
| const text = await aiGenerateNudge({ messages, user }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for AI generation calls.
The AI generation calls (aiGenerateNudge and aiGenerateReply) should be wrapped in try-catch blocks to handle potential failures gracefully.
Apply this diff to add error handling:
- const text = await aiGenerateNudge({ messages, user });
- return { text };
+ try {
+ const text = await aiGenerateNudge({ messages, user });
+ return { text };
+ } catch (error) {
+ console.error("Failed to generate nudge:", error);
+ return { error: "Failed to generate nudge. Please try again." };
+ }
- const text = await aiGenerateReply({ messages, user });
+ try {
+ const text = await aiGenerateReply({ messages, user });
+ } catch (error) {
+ console.error("Failed to generate reply:", error);
+ return { error: "Failed to generate reply. Please try again." };
+ }Also applies to: 82-83
| useEffect(() => { | ||
| async function loadNudge() { | ||
| const result = await generateNudgeAction({ | ||
| messages: [ | ||
| { | ||
| id: message.id, | ||
| textHtml: message.textHtml, | ||
| textPlain: message.textPlain, | ||
| date: message.headers.date, | ||
| from: message.headers.from, | ||
| to: message.headers.to, | ||
| subject: message.headers.subject, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| console.log("🚀 ~ result:", result); | ||
| } | ||
|
|
||
| if (generateNudge) loadNudge(); | ||
| }, [generateNudge, message]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and add error handling.
The implementation needs the following improvements:
- Remove the console.log statement.
- Add error handling for the nudge generation call.
Apply this diff to improve the implementation:
useEffect(() => {
async function loadNudge() {
- const result = await generateNudgeAction({
- messages: [
- {
- id: message.id,
- textHtml: message.textHtml,
- textPlain: message.textPlain,
- date: message.headers.date,
- from: message.headers.from,
- to: message.headers.to,
- subject: message.headers.subject,
- },
- ],
- });
+ try {
+ const result = await generateNudgeAction({
+ messages: [
+ {
+ id: message.id,
+ textHtml: message.textHtml,
+ textPlain: message.textPlain,
+ date: message.headers.date,
+ from: message.headers.from,
+ to: message.headers.to,
+ subject: message.headers.subject,
+ },
+ ],
+ });
- console.log("🚀 ~ result:", result);
+ if (result.error) {
+ // Handle error (e.g., show toast notification)
+ console.error("Failed to generate nudge:", result.error);
+ }
+ } catch (error) {
+ // Handle error (e.g., show toast notification)
+ console.error("Failed to generate nudge:", error);
+ }
}
if (generateNudge) loadNudge();
}, [generateNudge, message]);📝 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.
| useEffect(() => { | |
| async function loadNudge() { | |
| const result = await generateNudgeAction({ | |
| messages: [ | |
| { | |
| id: message.id, | |
| textHtml: message.textHtml, | |
| textPlain: message.textPlain, | |
| date: message.headers.date, | |
| from: message.headers.from, | |
| to: message.headers.to, | |
| subject: message.headers.subject, | |
| }, | |
| ], | |
| }); | |
| console.log("🚀 ~ result:", result); | |
| } | |
| if (generateNudge) loadNudge(); | |
| }, [generateNudge, message]); | |
| useEffect(() => { | |
| async function loadNudge() { | |
| try { | |
| const result = await generateNudgeAction({ | |
| messages: [ | |
| { | |
| id: message.id, | |
| textHtml: message.textHtml, | |
| textPlain: message.textPlain, | |
| date: message.headers.date, | |
| from: message.headers.from, | |
| to: message.headers.to, | |
| subject: message.headers.subject, | |
| }, | |
| ], | |
| }); | |
| if (result.error) { | |
| // Handle error (e.g., show toast notification) | |
| console.error("Failed to generate nudge:", result.error); | |
| } | |
| } catch (error) { | |
| // Handle error (e.g., show toast notification) | |
| console.error("Failed to generate nudge:", error); | |
| } | |
| } | |
| if (generateNudge) loadNudge(); | |
| }, [generateNudge, message]); |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/web/components/email-list/EmailDetails.tsx (1)
9-12: Consider improving date handling.The current date formatting might not handle invalid dates gracefully and doesn't consider user's timezone preferences.
Apply this diff to improve date handling:
- value: new Date(message.headers.date).toLocaleString(), + value: message.headers.date + ? new Date(message.headers.date).toLocaleString(undefined, { + dateStyle: "long", + timeStyle: "short", + }) + : "No date",apps/web/components/email-list/EmailContents.tsx (1)
43-67: Consider improving HTML injection safety.The
getIframeHtmlfunction should handle malformed HTML more safely.Consider using a proper HTML parser:
function getIframeHtml(html: string) { const defaultFontStyles = ` <style> /* Base styles with low specificity */ body { font-family: ui-sans-serif, system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Arial, sans-serif; margin: 0; } </style> `; + const parser = new DOMParser(); + const doc = parser.parseFromString(html, "text/html"); + const hasHead = doc.head !== null; + let htmlWithHead = ""; - if (html.indexOf("</head>") === -1) { + if (!hasHead) { htmlWithHead = `<head>${defaultFontStyles}<base target="_blank"></head>${html}`; } else { - htmlWithHead = html.replace( - "</head>", - `${defaultFontStyles}<base target="_blank" rel="noopener noreferrer"></head>`, - ); + doc.head.insertAdjacentHTML("beforeend", defaultFontStyles); + doc.head.insertAdjacentHTML("beforeend", '<base target="_blank" rel="noopener noreferrer">'); + htmlWithHead = doc.documentElement.outerHTML; } return htmlWithHead; }apps/web/components/email-list/EmailMessage.tsx (2)
51-59: Consider using a more robust scroll implementation.The current implementation uses a setTimeout which is noted as "hacky". A more reliable approach would be to use a callback after the component is mounted and rendered.
Consider this alternative implementation:
- useEffect(() => { - if (defaultShowReply && replyRef.current) { - setTimeout(() => { - replyRef.current?.scrollIntoView({ behavior: "smooth", block: "end" }); - // NOTE: a little hacky - // If this is set lower it doesn't work (or if we turn off autofocus, it does, but we want autofocus). - }, 500); - } - }, [defaultShowReply]); + const scrollToReply = useCallback(() => { + if (replyRef.current) { + replyRef.current.scrollIntoView({ behavior: "smooth", block: "end" }); + } + }, []); + + useEffect(() => { + if (defaultShowReply) { + // Use requestAnimationFrame to ensure DOM is updated + requestAnimationFrame(scrollToReply); + } + }, [defaultShowReply, scrollToReply]);
106-107: Add explanation for the biome-ignore comment.The biome-ignore comment for a11y/useKeyWithClickEvents should include an explanation of why the rule is being ignored.
-// biome-ignore lint/a11y/useKeyWithClickEvents: <explanation> +// biome-ignore lint/a11y/useKeyWithClickEvents: Keyboard events are handled by the parent component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/web/app/(app)/reply-tracker/ReplyTrackerEmails.tsx(1 hunks)apps/web/components/email-list/EmailAttachments.tsx(1 hunks)apps/web/components/email-list/EmailContents.tsx(1 hunks)apps/web/components/email-list/EmailDetails.tsx(1 hunks)apps/web/components/email-list/EmailMessage.tsx(1 hunks)apps/web/components/email-list/EmailThread.tsx(3 hunks)apps/web/components/email-list/types.ts(1 hunks)apps/web/utils/actions/generate-reply.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/components/email-list/EmailThread.tsx
- apps/web/utils/actions/generate-reply.ts
🔇 Additional comments (4)
apps/web/components/email-list/types.ts (1)
15-15: LGTM! Good type extraction pattern.The new
ThreadMessagetype is well-defined and improves code readability by extracting a specific type from theThreadtype.apps/web/components/email-list/EmailContents.tsx (1)
39-41: LGTM! Simple and effective implementation.The
PlainEmailcomponent is well-implemented with proper whitespace handling.apps/web/components/email-list/EmailMessage.tsx (1)
1-46: LGTM! Well-structured component props and imports.The component's props are well-typed and provide clear functionality for managing email message display and interaction.
apps/web/app/(app)/reply-tracker/ReplyTrackerEmails.tsx (1)
174-216: LGTM! Good handling of panel overflow.The implementation of independent scrolling for both panels using
h-full overflow-y-autois a good practice. This ensures that each panel can scroll its content independently while maintaining the overall layout structure.
| <Tooltip content="Forward"> | ||
| <Button variant="ghost" size="icon"> | ||
| <ForwardIcon className="h-4 w-4" onClick={onForward} /> | ||
| <span className="sr-only">Forward</span> | ||
| </Button> | ||
| </Tooltip> |
There was a problem hiding this comment.
Fix click handler placement on Forward button.
The onClick handler is incorrectly placed on the ForwardIcon instead of the Button component.
- <Tooltip content="Forward">
- <Button variant="ghost" size="icon">
- <ForwardIcon className="h-4 w-4" onClick={onForward} />
- <span className="sr-only">Forward</span>
- </Button>
- </Tooltip>
+ <Tooltip content="Forward">
+ <Button variant="ghost" size="icon" onClick={onForward}>
+ <ForwardIcon className="h-4 w-4" />
+ <span className="sr-only">Forward</span>
+ </Button>
+ </Tooltip>📝 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.
| <Tooltip content="Forward"> | |
| <Button variant="ghost" size="icon"> | |
| <ForwardIcon className="h-4 w-4" onClick={onForward} /> | |
| <span className="sr-only">Forward</span> | |
| </Button> | |
| </Tooltip> | |
| <Tooltip content="Forward"> | |
| <Button variant="ghost" size="icon" onClick={onForward}> | |
| <ForwardIcon className="h-4 w-4" /> | |
| <span className="sr-only">Forward</span> | |
| </Button> | |
| </Tooltip> |
| useEffect(() => { | ||
| async function loadNudge() { | ||
| const result = await generateNudgeAction({ | ||
| messages: [ | ||
| { | ||
| id: message.id, | ||
| textHtml: message.textHtml, | ||
| textPlain: message.textPlain, | ||
| date: message.headers.date, | ||
| from: message.headers.from, | ||
| to: message.headers.to, | ||
| subject: message.headers.subject, | ||
| }, | ||
| ], | ||
| }); | ||
|
|
||
| console.log("🚀 ~ result:", result); | ||
| } | ||
|
|
||
| if (generateNudge) loadNudge(); | ||
| }, [generateNudge, message]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove console.log and add error handling.
The nudge generation implementation needs two improvements:
- Remove the debug console.log statement
- Add error handling for the generateNudgeAction call
Consider this improved implementation:
useEffect(() => {
async function loadNudge() {
- const result = await generateNudgeAction({
- messages: [
- {
- id: message.id,
- textHtml: message.textHtml,
- textPlain: message.textPlain,
- date: message.headers.date,
- from: message.headers.from,
- to: message.headers.to,
- subject: message.headers.subject,
- },
- ],
- });
-
- console.log("🚀 ~ result:", result);
+ try {
+ const result = await generateNudgeAction({
+ messages: [
+ {
+ id: message.id,
+ textHtml: message.textHtml,
+ textPlain: message.textPlain,
+ date: message.headers.date,
+ from: message.headers.from,
+ to: message.headers.to,
+ subject: message.headers.subject,
+ },
+ ],
+ });
+ // Handle successful result
+ } catch (error) {
+ // Handle error appropriately
+ console.error('Failed to generate nudge:', error);
+ }
}
if (generateNudge) loadNudge();
}, [generateNudge, message]);📝 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.
| useEffect(() => { | |
| async function loadNudge() { | |
| const result = await generateNudgeAction({ | |
| messages: [ | |
| { | |
| id: message.id, | |
| textHtml: message.textHtml, | |
| textPlain: message.textPlain, | |
| date: message.headers.date, | |
| from: message.headers.from, | |
| to: message.headers.to, | |
| subject: message.headers.subject, | |
| }, | |
| ], | |
| }); | |
| console.log("🚀 ~ result:", result); | |
| } | |
| if (generateNudge) loadNudge(); | |
| }, [generateNudge, message]); | |
| useEffect(() => { | |
| async function loadNudge() { | |
| try { | |
| const result = await generateNudgeAction({ | |
| messages: [ | |
| { | |
| id: message.id, | |
| textHtml: message.textHtml, | |
| textPlain: message.textPlain, | |
| date: message.headers.date, | |
| from: message.headers.from, | |
| to: message.headers.to, | |
| subject: message.headers.subject, | |
| }, | |
| ], | |
| }); | |
| // Handle successful result | |
| } catch (error) { | |
| // Handle error appropriately | |
| console.error('Failed to generate nudge:', error); | |
| } | |
| } | |
| if (generateNudge) loadNudge(); | |
| }, [generateNudge, message]); |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/web/utils/actions/generate-reply.validation.ts (2)
3-15: Enhance schema validation rules for better data integrity.Consider adding these validations:
- Email format validation for
fromandtofields- Date format validation for the
datefield- Basic HTML content validation for
textHtmlconst messageSchema = z .object({ id: z.string(), - from: z.string(), - to: z.string(), + from: z.string().email(), + to: z.string().email(), subject: z.string(), textPlain: z.string().optional(), - textHtml: z.string().optional(), + textHtml: z.string() + .regex(/<[^>]*>/) + .optional(), - date: z.string(), + date: z.string() + .datetime() + .or(z.date()), })Also, consider adding JSDoc comments to document the schema fields:
/** Message schema for email reply generation * @property {string} id - Unique identifier for the message * @property {string} from - Sender's email address * @property {string} to - Recipient's email address * @property {string} subject - Email subject * @property {string} [textPlain] - Plain text content * @property {string} [textHtml] - HTML content * @property {string} date - Message date */
17-20: Add array length validation and documentation.Consider adding minimum length validation for the messages array and JSDoc comments.
+/** Schema for generating email replies and nudges + * @property {("reply"|"nudge")} type - Type of generation + * @property {Message[]} messages - Array of messages to process + */ export const generateReplySchema = z.object({ type: z.enum(["reply", "nudge"]), - messages: z.array(messageSchema), + messages: z.array(messageSchema).min(1, "At least one message is required"), });apps/web/components/email-list/EmailMessage.tsx (1)
210-217: Replace setTimeout with a more reliable solution.Using setTimeout for scroll behavior is fragile and may not work consistently across different devices and load times.
Consider using a MutationObserver or ResizeObserver for more reliable scroll behavior:
- useEffect(() => { - if (defaultShowReply && replyRef.current) { - // hacky using setTimeout - setTimeout(() => { - replyRef.current?.scrollIntoView({ behavior: "smooth", block: "end" }); - }, 500); - } - }, [defaultShowReply]); + useEffect(() => { + if (defaultShowReply && replyRef.current) { + const observer = new ResizeObserver(() => { + replyRef.current?.scrollIntoView({ behavior: "smooth", block: "end" }); + }); + observer.observe(replyRef.current); + return () => observer.disconnect(); + } + }, [defaultShowReply]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/components/email-list/EmailMessage.tsx(1 hunks)apps/web/utils/actions/generate-reply.ts(1 hunks)apps/web/utils/actions/generate-reply.validation.ts(1 hunks)apps/web/utils/ai/reply/generate-nudge.ts(1 hunks)apps/web/utils/ai/reply/generate-reply.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/utils/ai/reply/generate-nudge.ts
- apps/web/utils/actions/generate-reply.ts
- apps/web/utils/ai/reply/generate-reply.ts
🔇 Additional comments (2)
apps/web/utils/actions/generate-reply.validation.ts (1)
22-22: LGTM!The type export is well-defined and follows TypeScript best practices by inferring the type from the Zod schema.
apps/web/components/email-list/EmailMessage.tsx (1)
174-177: Fix click handler placement on Forward button.The onClick handler is incorrectly placed on the ForwardIcon instead of the Button component.
elie222
changed the title
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores