Generate prompt for user based on their history/labels#232
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new feature that enhances user interaction by adding a "loading" prop to various button components across multiple files. This prop replaces the previous conditional rendering of Changes
Possibly related PRs
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: 7
Outside diff range and nitpick comments (3)
apps/web/utils/mail.ts (1)
76-87: LGTM with suggestions for improvementThe changes to the
emailToContentfunction look good overall. The addition of the optionaloptionsparameter withmaxLengthprovides more flexibility in controlling the parsed email content length.Here are a few suggestions to further improve the code:
- Consider adding a return type annotation to the function for better type safety:
export function emailToContent( email: { textHtml: string | null; textPlain: string | null; snippet: string | null; }, options?: { maxLength: number } ): string { // ... function body ... }
- It might be beneficial to add validation for the
maxLengthvalue to ensure it's a positive number:const maxLength = options?.maxLength && options.maxLength > 0 ? options.maxLength : null; const content = (email.textHtml && parseEmail(email.textHtml, false, maxLength)) || email.textPlain || email.snippet;These changes will enhance the robustness and type safety of the function.
apps/web/app/(landing)/components/page.tsx (1)
64-66: Approve changes and suggest minor improvement for consistencyThe modification to use the
loadingprop directly on theShadButtoncomponent is a good improvement. It simplifies the code and is consistent with theButtoncomponent usage earlier in the file.For better consistency across the demo, consider adding loading state examples for other button variants as well. This would provide a more comprehensive showcase of the component's capabilities. For example:
<ShadButton variant="default" loading> ShadButton Default Loading </ShadButton> <ShadButton variant="secondary" loading> ShadButton Secondary Loading </ShadButton>apps/web/app/(app)/automation/RulesPrompt.tsx (1)
184-215: LGTM: AI Generate Prompt button implemented correctly.The new button and its associated functionality are well-implemented. The code follows React best practices, uses the existing toast notification system, and properly handles errors. The loading state is correctly managed, and the generated prompt is appropriately set to the form's state.
Consider extracting the onClick handler into a separate function for improved readability and maintainability. For example:
const handleGeneratePrompt = useCallback(async () => { toast.promise( async () => { setIsGenerating(true); const result = await generateRulesPromptAction(); setIsGenerating(false); if (isActionError(result)) throw new Error(result.error); if (!result) throw new Error("Unable to generate prompt"); return result; }, { loading: "Generating prompt...", success: (result) => { setValue("rulesPrompt", result.rulesPrompt); return "Prompt generated successfully!"; }, error: (err) => { return `Error generating prompt: ${err.message}`; }, }, ); }, [setValue]);Then, you can simplify the button's onClick prop:
onClick={handleGeneratePrompt}This change would improve code organization and make the component more readable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- apps/web/app/(app)/automation/RulesPrompt.tsx (3 hunks)
- apps/web/app/(landing)/components/page.tsx (1 hunks)
- apps/web/components/ui/button.tsx (3 hunks)
- apps/web/utils/actions/ai-rule.ts (4 hunks)
- apps/web/utils/ai/rule/generate-rules-prompt.ts (1 hunks)
- apps/web/utils/mail.ts (1 hunks)
Additional context used
Biome
apps/web/utils/actions/ai-rule.ts
[error] 714-714: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
apps/web/utils/ai/rule/generate-rules-prompt.ts
[error] 2-3: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
Additional comments not posted (10)
apps/web/utils/mail.ts (1)
76-87: Verify usage ofemailToContentin the codebaseTo ensure that the changes to
emailToContentdon't introduce any issues in the existing codebase, it's important to verify its usage. Please run the following script to check for all occurrences ofemailToContent:Review the output to ensure that all calls to
emailToContentare compatible with the new function signature. If any calls need to be updated to include the newoptionsparameter, please make the necessary changes.apps/web/components/ui/button.tsx (5)
4-4: LGTM: Loader2 import added correctlyThe import of
Loader2from 'lucide-react' is correctly added and necessary for the new loading state functionality.
36-38: LGTM: Loading variant added to buttonVariantsThe addition of the 'loading' variant to
buttonVariantsis well-implemented:
- It applies appropriate styles for a disabled state when loading is true.
- The default value of false for loading ensures backwards compatibility.
These changes effectively support the new loading state functionality.
Also applies to: 43-43
52-52: LGTM: Loading prop added to ButtonProps interfaceThe addition of the optional
loadingprop to theButtonPropsinterface is correct and well-typed. This allows users of the Button component to control the loading state, enhancing the component's flexibility.
56-67: LGTM: Loading prop correctly added to destructured propsThe
loadingprop is correctly added to the destructured props with a default value offalse. This ensures backwards compatibility and aligns with the interface definition. The placement among other props is appropriate.
Line range hint
1-94: Overall assessment: Well-implemented loading state with room for minor enhancementThe changes to the Button component effectively implement a loading state with good practices:
- The
buttonVariantsfunction andButtonPropsinterface are correctly updated.- The component handles the loading state appropriately, including visual feedback and disabling.
- The changes maintain backwards compatibility.
The suggestion to make children rendering optional during loading could further improve the component's flexibility. Consider implementing this enhancement in a future iteration.
Great work on this feature addition!
apps/web/utils/ai/rule/generate-rules-prompt.ts (1)
11-19: Function signature and type definitions look goodThe function signature is well-defined with appropriate TypeScript types. The use of destructuring in the parameter object improves readability, and the return type is correctly specified.
apps/web/app/(app)/automation/RulesPrompt.tsx (3)
11-14: LGTM: Import statement updated correctly.The addition of
generateRulesPromptActionimport is consistent with the new AI generation feature. The import statement is correctly structured and follows the existing pattern.
81-81: LGTM: New state variable added for AI generation.The addition of the
isGeneratingstate variable is appropriate for managing the loading state of the AI prompt generation process. The naming convention is consistent with the existing code style.
Line range hint
1-245: Overall implementation: Well-integrated and enhances functionality.The new AI prompt generation feature has been seamlessly integrated into the existing
RulesPromptFormcomponent. The implementation:
- Maintains the existing component structure and functionality.
- Follows consistent coding patterns and styles.
- Enhances the user experience by providing an AI-generated prompt option.
- Properly handles loading states and error scenarios.
The changes demonstrate a good balance between adding new functionality and maintaining code quality and consistency.
| export function emailToContent( | ||
| email: { | ||
| textHtml: string | null; | ||
| textPlain: string | null; | ||
| snippet: string | null; | ||
| }, | ||
| options?: { maxLength: number }, | ||
| ): string { | ||
| const content = | ||
| (email.textHtml && parseEmail(email.textHtml, false, null)) || | ||
| (email.textHtml && parseEmail(email.textHtml, false, options?.maxLength)) || | ||
| email.textPlain || | ||
| email.snippet; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding documentation and ensuring consistency
While the changes provide more flexibility, it's important to ensure clarity and consistency in usage. Consider the following suggestions:
- Add a comment to document the default behavior and the new option:
/**
* Converts an email object to a content string.
* @param email - The email object containing textHtml, textPlain, and snippet.
* @param options - Optional configuration object.
* @param options.maxLength - Maximum length for the parsed content. If not provided, defaults to 2000 characters.
* @returns The parsed and potentially truncated email content as a string.
*/
export function emailToContent(
// ... function signature ...
) {
// ... function body ...
}- Ensure consistency across the codebase by reviewing other places where
emailToContentis called. You may want to consider whether the defaultmaxLengthof 2000 characters (defined in theparseEmailfunction) is still appropriate for all use cases, or if it should be adjusted.
| className={cn(buttonVariants({ variant, size, loading, className }))} | ||
| ref={ref} | ||
| type={type} | ||
| disabled={loading || props.disabled} | ||
| {...props} | ||
| /> | ||
| > | ||
| {loading ? ( | ||
| <> | ||
| <Loader2 className="mr-2 h-4 w-4 animate-spin" /> | ||
| {children} | ||
| </> | ||
| ) : ( | ||
| children | ||
| )} | ||
| </Comp> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
LGTM with a minor suggestion: Button rendering updated for loading state
The changes to the Button component's rendering are well-implemented:
- The
buttonVariantsfunction correctly includes theloadingstate. - The button is properly disabled when either
loadingorprops.disabledis true. - The conditional rendering shows a
Loader2icon during the loading state, which is good for UX.
However, I have a minor suggestion:
Consider making the rendering of children in the loading state optional. Some use cases might prefer to show only the loading indicator. You could achieve this by adding a new prop, e.g., showChildrenWhileLoading:
export interface ButtonProps
extends React.ButtonHTMLAttributes<HTMLButtonElement>,
VariantProps<typeof buttonVariants> {
asChild?: boolean;
loading?: boolean;
+ showChildrenWhileLoading?: boolean;
}
const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(
({
className,
variant,
size,
asChild = false,
loading = false,
+ showChildrenWhileLoading = true,
children,
...props
},
ref,
) => {
// ... existing code ...
return (
<Comp
className={cn(buttonVariants({ variant, size, loading, className }))}
ref={ref}
type={type}
disabled={loading || props.disabled}
{...props}
>
{loading ? (
<>
<Loader2 className="mr-2 h-4 w-4 animate-spin" />
- {children}
+ {showChildrenWhileLoading && children}
</>
) : (
children
)}
</Comp>
);
},
);This change would provide more flexibility for different use cases while maintaining the current behavior by default.
Committable suggestion was skipped due to low confidence.
| const emailSummary = lastSentEmails | ||
| .map((email, index) => `Email ${index + 1}:\n${email}\n`) | ||
| .join("\n"); | ||
|
|
||
| const labelsList = userLabels.map((label) => `- ${label}`).join("\n"); | ||
|
|
||
| const system = | ||
| "You are an AI assistant that helps people manage their emails by generating rules based on their email behavior and existing labels."; | ||
|
|
||
| const prompt = ` | ||
| Analyze the user's email behavior and suggest general rules for managing their inbox effectively. Here's the context: | ||
|
|
||
| User Email: ${user.email} | ||
|
|
||
| Last 20 Sent Emails: | ||
| ${emailSummary} | ||
|
|
||
| User's Labels: | ||
| ${labelsList} | ||
|
|
||
| Generate a list of email management rules that would be broadly applicable for this user based on their email behavior and existing labels. The rules should be general enough to apply to various situations, not just specific recent emails. Include actions such as labeling, archiving, forwarding, replying, and drafting responses. Here are some examples of the format and complexity of rules you can create: | ||
|
|
||
| * Label newsletters as "Newsletter" and archive them | ||
| * If someone asks to schedule a meeting, send them your calendar link | ||
| * For cold emails or unsolicited pitches, draft a polite decline response | ||
| * Label emails related to financial matters as "Finance" and mark as important | ||
| * Forward emails about technical issues to the support team | ||
| * For emails from key clients or partners, label as "VIP" and keep in inbox | ||
|
|
||
| Focus on creating rules that will help the user organize their inbox more efficiently, save time, and automate responses where appropriate. Consider the following aspects: | ||
|
|
||
| 1. Labeling and organizing emails by general categories (e.g., Work, Personal, Finance) | ||
| 2. Handling common types of requests (e.g., meeting requests, support inquiries) | ||
| 3. Automating responses for recurring scenarios | ||
| 4. Forwarding specific types of emails to relevant team members | ||
| 5. Prioritizing important or urgent emails | ||
| 6. Dealing with newsletters, marketing emails, and potential spam | ||
|
|
||
| Your response should only include the list of general rules. Aim for 3-15 broadly applicable rules that would be useful for this user's email management. | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the long prompt into a separate constant or file
The prompt construction logic is clear and well-structured. However, the prompt is quite long and might benefit from being extracted into a separate constant or file for better maintainability.
Consider creating a separate file, e.g., promptTemplates.ts, to store this and other prompt templates. This would improve code organization and make it easier to manage and update prompts in the future.
Example:
// promptTemplates.ts
export const GENERATE_RULES_PROMPT = `
Analyze the user's email behavior and suggest general rules for managing their inbox effectively. Here's the context:
User Email: {{userEmail}}
Last 20 Sent Emails:
{{emailSummary}}
User's Labels:
{{labelsList}}
// ... rest of the prompt ...
`;
// In generate-rules-prompt.ts
import { GENERATE_RULES_PROMPT } from './promptTemplates';
// ...
const prompt = GENERATE_RULES_PROMPT
.replace('{{userEmail}}', user.email)
.replace('{{emailSummary}}', emailSummary)
.replace('{{labelsList}}', labelsList);| const aiResponse = await chatCompletionTools({ | ||
| userAi: user, | ||
| prompt, | ||
| system, | ||
| tools: { | ||
| generate_rules: { | ||
| description: "Generate a list of email management rules", | ||
| parameters, | ||
| }, | ||
| }, | ||
| userEmail: user.email, | ||
| label: "Generate rules prompt", | ||
| }); |
There was a problem hiding this comment.
Add error handling for AI interaction
The AI interaction logic looks good, but it lacks error handling. Consider adding try-catch blocks to handle potential errors during the AI interaction.
Here's a suggested implementation:
try {
const aiResponse = await chatCompletionTools({
userAi: user,
prompt,
system,
tools: {
generate_rules: {
description: "Generate a list of email management rules",
parameters,
},
},
userEmail: user.email,
label: "Generate rules prompt",
});
const parsedRules = aiResponse.toolCalls[0].args as z.infer<typeof parameters>;
return parsedRules.rules;
} catch (error) {
console.error("Error generating rules:", error);
throw new Error("Failed to generate email management rules");
}| const parsedRules = aiResponse.toolCalls[0].args as z.infer< | ||
| typeof parameters | ||
| >; | ||
|
|
||
| return parsedRules.rules; | ||
| } |
There was a problem hiding this comment.
Add validation for AI response and handle parsing errors
The current implementation assumes that the AI's response always matches the expected format and that the first tool call contains the rules. This might lead to runtime errors if the response is unexpected.
Consider adding validation and error handling:
const parsedResponse = parameters.safeParse(aiResponse.toolCalls[0]?.args);
if (!parsedResponse.success) {
console.error("Invalid AI response:", parsedResponse.error);
throw new Error("Failed to parse AI-generated rules");
}
return parsedResponse.data.rules;This approach uses Zod's safeParse method to validate the response and provides better error handling.
| import { z } from "zod"; | ||
| import { chatCompletionTools } from "@/utils/llms"; | ||
| import { type UserAIFields } from "@/utils/llms/types"; | ||
|
|
||
| const parameters = z.object({ | ||
| rules: z | ||
| .array(z.string()) | ||
| .describe("List of generated rules for email management"), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using import type for type-only imports
To improve code quality and ensure that types are removed during transpilation, consider using import type for imports that are only used as types.
Apply this diff to update the import statements:
-import { z } from "zod";
+import type { z } from "zod";
import { chatCompletionTools } from "@/utils/llms";
-import { type UserAIFields } from "@/utils/llms/types";
+import type { UserAIFields } from "@/utils/llms/types";Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 2-3: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
| const userLabels = await getLabels(gmail); | ||
| const lastSentMessages = await Promise.all( | ||
| lastSent.messages?.map(async (message) => { | ||
| const gmailMessage = await getMessage(message.id!, gmail); |
There was a problem hiding this comment.
Avoid using non-null assertion operator ! on message.id
Using the non-null assertion operator ! on message.id can lead to runtime errors if message.id is null or undefined. Consider adding a null check to ensure message.id is defined before using it.
Apply this diff to fix the issue:
- const gmailMessage = await getMessage(message.id!, gmail);
+ if (message.id) {
+ const gmailMessage = await getMessage(message.id, gmail);
+ return parseMessage(gmailMessage);
+ } else {
+ // Handle the case when message.id is undefined
+ // For example, log an error or skip processing this message
+ return null;
+ }Committable suggestion was skipped due to low confidence.
Tools
Biome
[error] 714-714: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores