Conversation
- Added `strip-indent` package (version 4.0.0) to the project dependencies. - Introduced `aiRuleFix` function to improve email rule handling based on provided email content. - Created a new test file for `aiRuleFix` to validate its functionality. - Updated `ReportMistake` component to utilize the new AI rule fixing logic. - Refactored `TestRules` and `TestResultDisplay` components to integrate the new reporting and rule fixing features. - Enhanced form validation for reporting mistakes in AI rules.
- Removed console log from `reportAiMistakeAction` for cleaner output. - Updated `reportAiMistakeAction` to include trimmed explanation in the result. - Modified `aiRuleFix` function to accept an optional explanation parameter and included it in the prompt for better context when fixing rules.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@elie222 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces several enhancements across multiple files, focusing on the implementation of a new 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: 6
🧹 Outside diff range and nitpick comments (11)
apps/web/utils/ai/rule/rule-fix.ts (1)
77-77: Reassess the necessity of passinguser.emailtochatCompletionObjectIf
user.emailis not essential for the function ofchatCompletionObject, consider omitting it to minimize handling of PII.apps/web/__tests__/ai-rule-fix.test.ts (1)
46-46: Removeconsole.logstatements from test codeConsider removing the
console.logstatement to keep test output clean. Tests should rely on assertions rather than logs.apps/web/app/(app)/automation/ReportMistake.tsx (2)
52-55: Remove commented-out codeConsider removing the commented-out console error statements to keep the codebase clean and maintainable.
100-110: Make theexplanationfield required and add validationSince the explanation helps in understanding the mistake, consider making it a required field and adding appropriate validation to ensure it is not empty.
Apply this diff to update the validation schema and form:
- import { - reportAiMistakeBody, - type ReportAiMistakeBody, - } from "@/utils/actions/validation"; + const reportAiMistakeFormSchema = reportAiMistakeBody.extend({ + explanation: z.string().min(1, "Explanation is required"), + }); + type ReportAiMistakeForm = z.infer<typeof reportAiMistakeFormSchema>; // In useForm: - } = useForm<ReportAiMistakeBody>({ - resolver: zodResolver(reportAiMistakeBody), + } = useForm<ReportAiMistakeForm>({ + resolver: zodResolver(reportAiMistakeFormSchema), // Update Input component to display required indicator <Input type="text" autosizeTextarea rows={3} name="explanation" label="Explanation" + required placeholder="What was incorrect about this response?" registerProps={register("explanation")} error={errors.explanation} />apps/web/app/(app)/cold-email-blocker/TestRules.tsx (4)
Line range hint
1-3: Consider extracting common components.The comment indicates this is a copy/paste of
automation/TestRules.tsx. Consider extracting shared components into a common location to avoid code duplication.
Line range hint
91-139: Enhance form validation and error handling.The form validation could be improved:
- Add input sanitization
- Add validation for email format
- Consider adding max length validation
const { register, handleSubmit, formState: { errors, isSubmitting }, } = useForm<TestRulesInputs>({ defaultValues: { message: "Hey, I run a development agency. I was wondering if you need extra hands on your team?", }, + validate: { + message: (value) => { + if (value.length > 1000) return "Message too long"; + if (!/\S+/.test(value)) return "Message cannot be empty"; + return true; + }, + }, });
Line range hint
108-126: Improve error handling in onSubmit callback.The error handling could be enhanced:
- Add error boundary
- Add retry mechanism for transient failures
- Clean up error logging
const onSubmit: SubmitHandler<TestRulesInputs> = useCallback(async (data) => { + try { const res = await postRequest< ColdEmailBlockerResponse, ColdEmailBlockerBody >("/api/ai/cold-email", { email: { from: "", subject: "", textHtml: null, textPlain: data.message, snippet: null, threadId: null, }, }); if (isError(res)) { - console.error(res); + // Log structured error for better debugging + console.error("Cold email check failed:", { + error: res.error, + message: data.message.substring(0, 100), // Log truncated message for context + }); toastError({ title: "Error checking if cold email.", description: res.error, }); } else { setColdEmailResponse(res); } + } catch (error) { + console.error("Unexpected error in cold email check:", error); + toastError({ + title: "Unexpected error", + description: "Please try again later", + }); + } }, []);
Line range hint
141-186: Potential memory leak in event handler.The click handler creates a new function on every render. Consider extracting it to a useCallback hook to prevent unnecessary re-renders and potential memory leaks.
+const handleTest = useCallback(async () => { + setLoading(true); + try { + const res = await postRequest<ColdEmailBlockerResponse, ColdEmailBlockerBody>( + "/api/ai/cold-email", + { + email: { + from: message.headers.from, + subject: message.headers.subject, + textHtml: message.textHtml || null, + textPlain: message.textPlain || null, + snippet: message.snippet || null, + threadId: message.threadId, + }, + } + ); + if (isError(res)) { + console.error(res); + toastError({ + title: "Error checking whether it's a cold email.", + description: res.error, + }); + } else { + setColdEmailResponse(res); + } + } finally { + setLoading(false); + } +}, [message, setColdEmailResponse]);apps/web/app/(app)/automation/TestRules.tsx (1)
Line range hint
251-266: Improve accessibility for alert components.Add ARIA labels and roles to enhance screen reader support.
- <Alert variant="destructive"> + <Alert + variant="destructive" + role="alert" + aria-live="assertive" + >- <Alert variant="blue"> + <Alert + variant="blue" + role="status" + aria-live="polite" + >Also applies to: 296-322
apps/web/utils/actions/ai-rule.ts (2)
884-885: Address the TODO comment and improve error handling.The TODO comment indicates uncertainty about handling missing rule instructions. Consider implementing a more specific error message that guides the user on how to resolve the issue.
Would you like me to help implement a more robust error handling strategy for this case?
869-920: Consider adding logging for error cases.The action would benefit from logging error cases to help with debugging and monitoring. Consider adding logging statements for:
- Input validation failures
- Missing rule cases
- AI rule fixing failures
export const reportAiMistakeAction = withActionInstrumentation( "reportAiMistake", async (unsafeBody: ReportAiMistakeBody) => { const session = await auth(); if (!session?.user.id) return { error: "Not logged in" }; const { success, data, error } = reportAiMistakeBody.safeParse(unsafeBody); - if (!success) return { error: error.message }; + if (!success) { + logger.error("Input validation failed", { error: error.message }); + return { error: error.message }; + } const { ruleId, email, explanation } = data; if (!ruleId) return { error: "Rule ID is required" }; const rule = await prisma.rule.findUnique({ where: { id: ruleId, userId: session.user.id }, }); - if (!rule?.instructions) return { error: "No instructions for rule" }; + if (!rule?.instructions) { + logger.error("Missing rule instructions", { ruleId }); + return { error: "No instructions found for the specified rule" }; + } // ... rest of the implementation - if (isActionError(result)) return { error: result.error }; - if (!result) return { error: "Error fixing rule" }; + if (isActionError(result)) { + logger.error("AI rule fixing failed", { error: result.error }); + return { error: result.error }; + } + if (!result) { + logger.error("AI rule fixing returned null result"); + return { error: "Failed to generate fixed instructions" }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/web/__tests__/ai-rule-fix.test.ts(1 hunks)apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/app/(app)/automation/TestRules.tsx(8 hunks)apps/web/app/(app)/cold-email-blocker/TestRules.tsx(1 hunks)apps/web/components/SearchForm.tsx(2 hunks)apps/web/package.json(1 hunks)apps/web/utils/actions/ai-rule.ts(3 hunks)apps/web/utils/actions/validation.ts(1 hunks)apps/web/utils/ai/rule/rule-fix.ts(1 hunks)
🔇 Additional comments (7)
apps/web/components/SearchForm.tsx (1)
44-44: LGTM! Button variant change is appropriate
The change from color="transparent" to variant="ghost" follows common component library patterns while maintaining the same visual effect. The loading state is properly handled.
apps/web/utils/actions/validation.ts (1)
95-107: Validation schema added correctly
The reportAiMistakeBody schema is well-defined and appropriately validates the required fields for reporting AI mistakes.
apps/web/package.json (1)
119-119: LGTM: Added strip-indent dependency.
The added dependency is at its latest version and is actively maintained.
apps/web/app/(app)/automation/TestRules.tsx (1)
38-38: LGTM: Good type definition.
Extracting the Message type improves code readability and maintainability.
apps/web/utils/actions/ai-rule.ts (3)
32-33: LGTM! Import statements are properly organized.
The new imports for report mistake functionality are correctly placed and properly typed.
Also applies to: 45-45
905-914: LGTM! Proper error handling and type safety.
The implementation correctly:
- Passes typed parameters to
aiRuleFix - Trims the explanation text
- Makes explanation optional with undefined fallback
899-903: Consider handling empty content case.
The emailToContent call should handle the case where all content fields (textHtml, textPlain, and snippet) are empty or null.
| import { type SubmitHandler, useForm } from "react-hook-form"; | ||
| import { SearchIcon } from "lucide-react"; | ||
| import { Button } from "@/components/Button"; | ||
| import { Button } from "@/components/ui/button"; |
There was a problem hiding this comment.
💡 Codebase verification
Several files still use the old Button import path
The verification revealed inconsistent Button import paths across the codebase. Multiple files are still using the old import path @/components/Button:
apps/web/components/TopNav.tsxapps/web/components/PremiumAlert.tsxapps/web/components/GroupHeading.tsxapps/web/components/ErrorPage.tsxapps/web/components/Celebration.tsx- And 19 other files in the
apps/webdirectory
The import path should be updated to @/components/ui/button to maintain consistency with the new component structure.
🔗 Analysis chain
Verify consistent Button import paths across the codebase
The import path change follows good practices by organizing UI components under a dedicated directory.
Let's verify the consistency of Button imports across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old Button import paths
# Expect: All Button imports should use the new path
# Search for old import pattern
echo "Checking for old Button import pattern..."
rg "from ['\"]@/components/Button['\"]"
# Search for new import pattern for reference
echo "Checking new Button import pattern..."
rg "from ['\"]@/components/ui/button['\"]"
Length of output: 10601
| const aiResponse = await chatCompletionObject({ | ||
| userAi: user, | ||
| prompt, | ||
| system, | ||
| schema: ruleFixSchema, | ||
| userEmail: user.email ?? "", | ||
| usageLabel: "ai-rule-fix", | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for chatCompletionObject to improve robustness
The asynchronous call to chatCompletionObject may throw exceptions. Adding error handling will enhance stability and prevent unhandled promise rejections.
Apply this diff to implement error handling:
logger.trace({ system, prompt });
+ try {
const aiResponse = await chatCompletionObject({
userAi: user,
prompt,
system,
schema: ruleFixSchema,
userEmail: user.email ?? "",
usageLabel: "ai-rule-fix",
});
logger.trace(aiResponse.object);
return aiResponse.object;
+ } catch (error) {
+ logger.error('Error in aiRuleFix:', error);
+ throw error; // Handle the error appropriately
+ }Committable suggestion skipped: line range outside the PR's diff.
| alert( | ||
| "No rule found. Can't report mistake. Will be implemented in the future.", | ||
| ); | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using alert for user notifications
Using alert can disrupt the user experience and is discouraged in modern web applications. Consider utilizing a toast notification or modal to provide feedback.
Apply this diff to replace alert with a toast notification:
if (!data?.ruleId) {
- alert(
- "No rule found. Can't report mistake. Will be implemented in the future.",
- );
+ toastError({
+ title: "Reporting Unavailable",
+ description:
+ "No rule found. Can't report mistake. This feature will be implemented in the future.",
+ });
return;
}📝 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.
| alert( | |
| "No rule found. Can't report mistake. Will be implemented in the future.", | |
| ); | |
| return; | |
| toastError({ | |
| title: "Reporting Unavailable", | |
| description: | |
| "No rule found. Can't report mistake. This feature will be implemented in the future.", | |
| }); | |
| return; |
| function TestResultDisplay({ | ||
| result, | ||
| message, | ||
| }: { | ||
| result: TestResult; | ||
| message: Message | null; | ||
| }) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety for message parameter.
Consider making the message parameter required when there's a rule match, as the ReportMistake component expects a non-null message.
-function TestResultDisplay({
- result,
- message,
-}: {
- result: TestResult;
- message: Message | null;
-}) {
+type TestResultDisplayProps =
+ | { result: TestResult & { rule: null }; message: Message | null }
+ | { result: TestResult & { rule: { name: string } }; message: Message };
+
+function TestResultDisplay(props: TestResultDisplayProps) {
+ const { result, message } = props;📝 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.
| function TestResultDisplay({ | |
| result, | |
| message, | |
| }: { | |
| result: TestResult; | |
| message: Message | null; | |
| }) { | |
| type TestResultDisplayProps = | |
| | { result: TestResult & { rule: null }; message: Message | null } | |
| | { result: TestResult & { rule: { name: string } }; message: Message }; | |
| function TestResultDisplay(props: TestResultDisplayProps) { | |
| const { result, message } = props; |
- Updated the `saveAboutBody` schema to enforce a maximum length of 2000 characters for the `about` field, improving data integrity and user input validation.
- Enhanced the `saveAboutAction` and `saveOnboardingAnswersAction` functions to include safe parsing of input data using Zod schemas, ensuring better validation and error handling. - Updated the parameter names for clarity, changing `options` to `unsafeBody` in `saveAboutAction` and similarly in `saveOnboardingAnswersAction`. - Improved error responses to return specific error messages when validation fails, enhancing user feedback.
- Introduced a new Zod schema `testAiBody` for validating input in the `testAiAction`, ensuring proper structure and error handling. - Updated the `testAiAction` to accept a structured input object instead of separate parameters, improving clarity and maintainability. - Modified the `ReportMistake` component's placeholder text to provide clearer guidance for users on what to include in their feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/web/utils/actions/validation.ts (1)
101-111: Consider strengthening validation rulesThe email object's validation could be enhanced:
textHtmlandtextPlainare both optional - consider requiring at least oneexplanationbeing optional might miss valuable user feedbackexport const reportAiMistakeBody = z.object({ email: z.object({ from: z.string(), subject: z.string(), snippet: z.string(), - textHtml: z.string().nullish(), - textPlain: z.string().nullish(), + textHtml: z.string().optional(), + textPlain: z.string().optional(), }).refine( + data => data.textHtml || data.textPlain, + "At least one of textHtml or textPlain must be provided" ), ruleId: z.string().nullish(), - explanation: z.string().nullish(), + explanation: z.string().min(1, "Please provide an explanation"), });apps/web/utils/actions/ai-rule.ts (2)
891-892: Address the TODO comment regarding error handling.The comment "TODO: how should we handle this?" needs to be addressed. Consider implementing proper error handling for cases where the rule exists but has no instructions.
Would you like me to help implement a more robust error handling strategy for this case? We could:
- Log the incident for monitoring
- Return a more specific error message
- Potentially create a new rule if the existing one is invalid
876-927: Consider adding transaction handling and logging.The function could benefit from:
- Transaction handling to ensure data consistency
- Logging for better debugging and monitoring
- More specific error messages
Here's a suggested improvement:
export const reportAiMistakeAction = withActionInstrumentation( "reportAiMistake", async (unsafeBody: ReportAiMistakeBody) => { + const logger = createScopedLogger("reportAiMistake"); const session = await auth(); if (!session?.user.id) return { error: "Not logged in" }; const { success, data, error } = reportAiMistakeBody.safeParse(unsafeBody); if (!success) return { error: error.message }; const { ruleId, email, explanation } = data; - if (!ruleId) return { error: "Rule ID is required" }; + if (!ruleId) { + logger.error("Rule ID missing in request"); + return { error: "Rule ID is required to report an AI mistake" }; + } - const rule = await prisma.rule.findUnique({ - where: { id: ruleId, userId: session.user.id }, - }); - if (!rule?.instructions) return { error: "No instructions for rule" }; + return await prisma.$transaction(async (tx) => { + const rule = await tx.rule.findUnique({ + where: { id: ruleId, userId: session.user.id }, + }); + + if (!rule) { + logger.error("Rule not found", { ruleId }); + return { error: "Rule not found. It might have been deleted." }; + } + + if (!rule.instructions) { + logger.error("Rule has no instructions", { ruleId }); + return { error: "Cannot fix rule: missing instructions" }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/app/api/google/messages/route.ts(1 hunks)apps/web/utils/actions/ai-rule.ts(4 hunks)apps/web/utils/actions/user.ts(2 hunks)apps/web/utils/actions/validation.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(app)/automation/ReportMistake.tsx
🔇 Additional comments (10)
apps/web/app/api/google/messages/route.ts (4)
15-15: LGTM: Improved authentication check
The more specific check for session?.user.email enhances security by ensuring the email is present.
28-33: LGTM: Enhanced null safety and filtering
Good addition of null checks and the use of isDefined filter to handle potential undefined messages safely.
21-21: Verify impact of increased maxResults
Increasing maxResults from 10 to 20 could impact performance and rate limits.
Consider implementing pagination if you need to process more messages, rather than increasing the batch size.
35-38: Verify the email filtering logic
The comment suggests that -from:me wasn't working as expected. The new filtering approach looks correct but should be tested thoroughly.
Would you like me to help create test cases for this filtering logic?
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to find any related test files and understand the implementation context better. This will help us verify if the email filtering logic is properly tested or if there are potential issues.
Let's examine the complete implementation file and any dependencies to better understand the email filtering logic and its usage context.
Email filtering logic is correctly implemented
The implementation is sound for the following reasons:
- The comment accurately explains why
-from:mequery parameter wasn't suitable (it would filter out entire threads where the user responded) - The current approach correctly filters messages by checking the sender's email against the authenticated user's email
- The filtering is applied after fetching the full message details, which ensures accurate sender information
- The implementation is used across multiple components (email panel, test rules, compose form) suggesting it's working as expected in production
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for related tests
rg -l 'describe.*getMessages|test.*getMessages|it.*getMessages' -g '*.test.*'
Length of output: 79
Script:
#!/bin/bash
# Let's look for any tests related to Google messages or email filtering
rg -l "google.*messages|email.*filter" -g "*.test.*"
# Also search for the actual implementation to understand the context better
rg -A 5 "fullMessages.filter.*message.*headers.*from"
Length of output: 110
Script:
#!/bin/bash
# Let's look at the entire route file to understand the context better
cat "apps/web/app/api/google/messages/route.ts"
# Also search for any files that might be importing or using this route
rg -l "api/google/messages"
Length of output: 2125
apps/web/utils/actions/validation.ts (1)
95-99: LGTM: Clear and focused test schema
The schema correctly validates required message and thread IDs.
apps/web/utils/actions/user.ts (2)
17-17: LGTM: Added maximum length constraint
Good addition of max length constraint (2000 chars) for the about field.
106-109: LGTM: Consistent error handling
Good use of safeParse for validation with clear error messages.
apps/web/utils/actions/ai-rule.ts (3)
32-37: LGTM! New imports are well-organized.
The new imports for validation schemas, types, and the aiRuleFix utility are properly structured and align with the new functionality being added.
Also applies to: 47-47
912-921: LGTM! AI rule fix integration looks good.
The integration with aiRuleFix is well-structured:
- Passes all required parameters
- Properly handles the explanation field
- Maintains type safety
906-911: Consider handling empty content edge case.
The email content processing should handle the case where all content fields (textHtml, textPlain, and snippet) are empty or null.
Let's verify if this edge case is handled properly:
| const saveOnboardingAnswersBody = z.object({ | ||
| surveyId: z.string().optional(), | ||
| questions: z.any(), | ||
| answers: z.any(), | ||
| }); |
There was a problem hiding this comment.
Improve type safety in onboarding schema
The schema uses z.any() for questions and answers, which bypasses TypeScript's type checking.
Consider defining specific types for questions and answers:
const saveOnboardingAnswersBody = z.object({
surveyId: z.string().optional(),
- questions: z.any(),
- answers: z.any(),
+ questions: z.array(z.object({
+ id: z.string(),
+ text: z.string(),
+ type: z.enum(['text', 'multiple_choice', 'checkbox']),
+ })),
+ answers: z.record(z.string(), z.union([z.string(), z.array(z.string())])),
});📝 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 saveOnboardingAnswersBody = z.object({ | |
| surveyId: z.string().optional(), | |
| questions: z.any(), | |
| answers: z.any(), | |
| }); | |
| const saveOnboardingAnswersBody = z.object({ | |
| surveyId: z.string().optional(), | |
| questions: z.array(z.object({ | |
| id: z.string(), | |
| text: z.string(), | |
| type: z.enum(['text', 'multiple_choice', 'checkbox']), | |
| })), | |
| answers: z.record(z.string(), z.union([z.string(), z.array(z.string())])), | |
| }); |
…unctionality - Reduced the number of rows in the ReportMistake input field from 3 to 2 for a more compact design. - Adjusted the minimum width of the key label in TestResultDisplay from 20 to 16 for better alignment. - Added a maximum height and overflow handling to the value display in TestResultDisplay, enhancing readability for longer text.
elie222
force-pushed
the
report-mistake
branch
from
decd390 to
47a7f1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
apps/web/components/HoverCard.tsx (1)
7-11: LGTM! Consider using ComponentProps for better type safety.The addition of the optional className prop and its implementation is clean and improves component reusability.
Consider using ComponentProps from React for better type safety:
+import type { ComponentProps } from "react"; export function HoverCard(props: { children: React.ReactNode; content: React.ReactNode; - className?: string; + className?: ComponentProps<typeof HoverCardContent>["className"]; })Also applies to: 15-15
apps/web/app/(app)/automation/ExecutedRulesTable.tsx (1)
71-73: LGTM! Consider extracting the width class as a constant.The width class addition is appropriate for the hover content.
Consider extracting the width class to a shared constant since it's used across multiple components:
// constants/styles.ts export const HOVER_CARD_WIDTH_CLASS = "w-80" as const; // Usage import { HOVER_CARD_WIDTH_CLASS } from "@/constants/styles"; <HoverCard className={HOVER_CARD_WIDTH_CLASS} />apps/web/utils/actions/validation.ts (1)
101-121: Consider adding additional validation constraints.While the schema structure is good, consider adding:
- Email format validation for the
fromfield- Maximum length constraints for text fields to prevent excessive data
- Minimum length constraints for the explanation field when provided
export const reportAiMistakeBody = z .object({ email: z.object({ - from: z.string(), + from: z.string().email(), - subject: z.string(), + subject: z.string().max(500), - snippet: z.string(), + snippet: z.string().max(1000), textHtml: z.string().nullish(), textPlain: z.string().nullish(), }), correctRuleId: z.string().nullish(), incorrectRuleId: z.string().nullish(), - explanation: z.string().nullish(), + explanation: z.string().min(10).max(1000).nullish(), })apps/web/utils/ai/rule/rule-fix.ts (1)
94-167: Consider refactoring for better maintainabilityThe function could be simplified by:
- Extracting the schema and examples into separate constants
- Using a map or switch statement instead of multiple if blocks
+const RULE_FIX_SCHEMAS = { + BOTH_RULES: z.object({ + rule: z.enum(["matched_rule", "correct_rule"]), + fixedInstructions: z.string().describe("The updated instructions"), + }), + SINGLE_RULE: z.object({ + fixedInstructions: z.string().describe("The updated instructions"), + }), +}; +const RULE_FIX_EXAMPLES = { + BOTH_RULES: [ + stripIndent(`{...}`), // existing examples + ], + SINGLE_RULE: [ + stripIndent(`{...}`), // existing examples + ], +}; function getRuleFixPromptConfig( incorrectRule: Pick<Rule, "instructions"> | null, correctRule: Pick<Rule, "instructions"> | null, ) { + const config = { + INCORRECT_AND_CORRECT: { + problem: stripIndent(`...`), + schema: RULE_FIX_SCHEMAS.BOTH_RULES, + examples: RULE_FIX_EXAMPLES.BOTH_RULES, + }, + // ... other configurations + }; + + const key = getConfigKey(incorrectRule, correctRule); + const result = config[key]; + + if (!result) throw new Error("No rule to fix"); + return result; }apps/web/app/(app)/automation/ReportMistake.tsx (2)
146-148: Remove or implement error logging properlyThe commented-out error logging should either be removed or properly implemented with a logging service.
- // if (Object.keys(errors).length > 0) { - // console.error("Errors:", errors); - // }Also applies to: 212-214
150-164: Consider implementing optimistic updatesThe form submission handlers could benefit from optimistic updates to improve the user experience. This would involve:
- Immediately updating the UI
- Rolling back changes if the request fails
Example implementation for updateRule:
const updateRule: SubmitHandler<UpdateRuleBody> = useCallback( async (data) => { // Store previous value const previousValue = rule.instructions; try { // Optimistically update UI // Update local state or invalidate SWR cache const response = await updateRuleAction(data); if (isActionError(response)) { throw new Error(response.error); } toastSuccess({ description: "Rule updated!" }); } catch (error) { // Rollback on error // Restore previous value toastError({ title: "Error updating rule", description: error.message, }); } }, [rule.instructions] );Also applies to: 216-242
apps/web/utils/actions/ai-rule.ts (1)
934-943: Consider improving clarity and null handling.
- The ternary operator for determining the returned ruleId might be confusing. Consider making it more explicit:
- The explanation trimming should check for null/undefined first.
- explanation: explanation?.trim() || undefined, + explanation: explanation ? explanation.trim() : undefined, + }); + + if (isActionError(result)) return { error: result.error }; + if (!result) return { error: "Error fixing rule" }; + + const ruleId = result.rule === "matched_rule" + ? incorrectRuleId // Rule matched incorrectly + : correctRuleId; // Rule matched correctly + + return { + ruleId, + fixedInstructions: result.fixedInstructions,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
apps/web/__tests__/ai-rule-fix.test.ts(1 hunks)apps/web/app/(app)/automation/ExecutedRulesTable.tsx(1 hunks)apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/app/(app)/automation/TestRules.tsx(8 hunks)apps/web/app/api/google/messages/route.ts(1 hunks)apps/web/components/HoverCard.tsx(1 hunks)apps/web/components/PlanBadge.tsx(2 hunks)apps/web/utils/actions/ai-rule.ts(4 hunks)apps/web/utils/actions/validation.ts(1 hunks)apps/web/utils/ai/rule/rule-fix.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/components/PlanBadge.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/tests/ai-rule-fix.test.ts
- apps/web/app/api/google/messages/route.ts
- apps/web/app/(app)/automation/TestRules.tsx
🔇 Additional comments (6)
apps/web/utils/actions/validation.ts (1)
95-99: LGTM: Schema definition is clear and appropriate.
The schema correctly defines the required fields for testing AI functionality.
apps/web/utils/ai/rule/rule-fix.ts (2)
46-46: Avoid including user's 'about' information in AI prompts to prevent PII exposure
Including user.about in the AI prompt may expose sensitive personal information.
71-79: Add error handling for chatCompletionObject to improve robustness
The asynchronous call lacks error handling which could lead to unhandled promise rejections.
apps/web/app/(app)/automation/ReportMistake.tsx (1)
221-224: Avoid using alert for user notifications
Using alert can disrupt the user experience and is discouraged in modern web applications.
apps/web/utils/actions/ai-rule.ts (2)
112-119: LGTM! Good improvements to input validation.
The changes enhance type safety and error handling by adding proper validation of the input parameters.
876-945: LGTM! Well-structured implementation with good practices.
The implementation includes:
- Proper input validation
- Concurrent data fetching
- User ownership validation
- Clear error messages
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
apps/web/utils/actions/validation.ts (1)
87-93: Consider adding length constraints to the instructions field.The
instructionsfield inupdateRuleInstructionsBodyshould have length constraints to prevent excessively long inputs that could impact performance or storage.export const updateRuleInstructionsBody = z.object({ id: z.string(), - instructions: z.string(), + instructions: z.string().min(1).max(5000), });apps/web/app/(app)/automation/ReportMistake.tsx (2)
265-267: Improve loading state feedback.The "Fix with AI" button shows a loading state, but there's no indication of progress or what's happening during the AI processing.
- <Button type="submit" loading={isSubmitting}> - Fix with AI - </Button> + <Button type="submit" loading={isSubmitting}> + {isSubmitting ? "Analyzing with AI..." : "Fix with AI"} + </Button>
68-70: Uncomment and update the dialog description.The commented dialog description would help users understand the feature better.
- {/* <DialogDescription> - Explain what went wrong and our AI will suggest a fix. - </DialogDescription> */} + <DialogDescription> + Select the expected rule and explain what went wrong. Our AI will suggest improvements to make the rules more accurate. + </DialogDescription>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/utils/actions/ai-rule.ts(4 hunks)apps/web/utils/actions/rule.ts(2 hunks)apps/web/utils/actions/validation.ts(2 hunks)
🔇 Additional comments (3)
apps/web/app/(app)/automation/ReportMistake.tsx (1)
240-246: Address the TODO comment about undefined ruleId.
The TODO comment indicates uncertainty about when ruleId could be undefined. This should be investigated and properly handled.
Would you like me to help investigate when ruleId could be undefined and implement proper error handling?
apps/web/utils/actions/ai-rule.ts (2)
112-119: Great addition of input validation!
The changes improve security by validating the input data before processing, using proper error handling.
876-945: Well-structured implementation with proper validation and error handling!
The action effectively handles AI mistakes with proper input validation, concurrent data fetching, and comprehensive error handling.
| export const reportAiMistakeAction = withActionInstrumentation( | ||
| "reportAiMistake", | ||
| async (unsafeBody: ReportAiMistakeBody) => { | ||
| const session = await auth(); | ||
| if (!session?.user.id) return { error: "Not logged in" }; | ||
|
|
||
| const { success, data, error } = reportAiMistakeBody.safeParse(unsafeBody); | ||
| if (!success) return { error: error.message }; | ||
| const { correctRuleId, incorrectRuleId, email, explanation } = data; | ||
|
|
||
| if (!correctRuleId && !incorrectRuleId) | ||
| return { error: "Either correct or incorrect rule ID is required" }; | ||
|
|
||
| const [correctRule, incorrectRule, user] = await Promise.all([ | ||
| correctRuleId | ||
| ? prisma.rule.findUnique({ | ||
| where: { id: correctRuleId, userId: session.user.id }, | ||
| }) | ||
| : null, | ||
| incorrectRuleId | ||
| ? prisma.rule.findUnique({ | ||
| where: { id: incorrectRuleId, userId: session.user.id }, | ||
| }) | ||
| : null, | ||
| prisma.user.findUnique({ | ||
| where: { id: session.user.id }, | ||
| select: { | ||
| email: true, | ||
| about: true, | ||
| aiProvider: true, | ||
| aiModel: true, | ||
| aiApiKey: true, | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| if (correctRuleId && !correctRule) | ||
| return { error: "Correct rule not found" }; | ||
|
|
||
| if (incorrectRuleId && !incorrectRule) | ||
| return { error: "Incorrect rule not found" }; | ||
|
|
||
| if (!user) return { error: "User not found" }; | ||
|
|
||
| const content = emailToContent({ | ||
| textHtml: email.textHtml || null, | ||
| textPlain: email.textPlain || null, | ||
| snippet: email.snippet, | ||
| }); | ||
|
|
||
| const result = await aiRuleFix({ | ||
| user, | ||
| incorrectRule, | ||
| correctRule, | ||
| email: { | ||
| ...email, | ||
| content, | ||
| }, | ||
| explanation: explanation?.trim() || undefined, | ||
| }); | ||
|
|
||
| if (isActionError(result)) return { error: result.error }; | ||
| if (!result) return { error: "Error fixing rule" }; | ||
|
|
||
| return { | ||
| ruleId: result.rule === "matched_rule" ? incorrectRuleId : correctRuleId, | ||
| fixedInstructions: result.fixedInstructions, | ||
| }; | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding transaction handling and rate limiting.
To improve reliability and security:
- Wrap database operations in a transaction to ensure data consistency
- Add rate limiting to prevent potential abuse of the AI service
- Enhance error messages to be more descriptive for better debugging
Example implementation:
export const reportAiMistakeAction = withActionInstrumentation(
"reportAiMistake",
async (unsafeBody: ReportAiMistakeBody) => {
const session = await auth();
if (!session?.user.id) return { error: "Not logged in" };
+ // Add rate limiting
+ const rateLimitKey = `report-ai-mistake:${session.user.id}`;
+ const rateLimit = await getRateLimit(rateLimitKey);
+ if (!rateLimit.success) {
+ return { error: "Too many requests. Please try again later." };
+ }
const { success, data, error } = reportAiMistakeBody.safeParse(unsafeBody);
if (!success) return { error: error.message };
const { correctRuleId, incorrectRuleId, email, explanation } = data;
if (!correctRuleId && !incorrectRuleId)
- return { error: "Either correct or incorrect rule ID is required" };
+ return { error: "Please provide either the correct rule ID or the incorrect rule ID that was applied to this email" };
+ // Wrap operations in a transaction
+ return await prisma.$transaction(async (tx) => {
const [correctRule, incorrectRule, user] = await Promise.all([
correctRuleId
? prisma.rule.findUnique({
where: { id: correctRuleId, userId: session.user.id },
})
: null,
// ... rest of the implementation
]);
+ });
}
);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/web/app/(app)/automation/ReportMistake.tsx (1)
270-270: Address the TODO: Determine whenruleIdcan be undefinedThere is a TODO comment questioning when
ruleIdcould be undefined. Clarifying this can prevent potential runtime errors.Would you like assistance in identifying cases where
ruleIdmight be undefined?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/app/(app)/automation/TestRules.tsx(7 hunks)
🔇 Additional comments (1)
apps/web/app/(app)/automation/ReportMistake.tsx (1)
253-255: Remove debug console.error statements
Debug console statements should not be present in production code. Consider using proper error logging or monitoring services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/web/app/(app)/automation/ReportMistake.tsx (5)
72-74: Remove commented out codeRemove the commented DialogDescription if it's not needed. If it might be needed later, consider tracking it in a TODO ticket instead.
- {/* <DialogDescription> - Explain what went wrong and our AI will suggest a fix. - </DialogDescription> */}
57-58: Consider combining related statesThe
checkingandtestResultstates are closely related. Consider combining them into a single state object for better state management.- const [checking, setChecking] = useState(false); - const [testResult, setTestResult] = useState<TestResult>(); + const [testState, setTestState] = useState<{ + isChecking: boolean; + result?: TestResult; + }>({ isChecking: false });
277-278: Clarify TODO comment with specific scenariosThe TODO comment about undefined ruleId needs more context. Consider documenting specific scenarios when this could happen or add validation to prevent undefined cases.
- // TODO: when is ruleId undefined? + // TODO: Add validation for undefined ruleId. This can happen when: + // 1. <document specific scenarios>
192-206: Extract common form submission logic into a custom hookBoth
updateRuleandreportMistakehandlers share similar error handling patterns. Consider extracting this logic into a custom hook for better reusability.Example implementation:
function useFormSubmission<T>( action: (data: T) => Promise<ActionResponse>, successMessage: string ) { return useCallback(async (data: T) => { const response = await action(data); if (isActionError(response)) { toastError({ title: "Error", description: response.error, }); } else { toastSuccess({ description: successMessage }); return response; } }, []); }Also applies to: 264-287
329-329: Use design system tokens for colorsInstead of using hardcoded color values, consider using design system tokens for consistency.
- <div className="mt-2 rounded border border-gray-200 bg-gray-50 p-2 text-sm"> + <div className="mt-2 rounded border border-border bg-muted p-2 text-sm">
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
apps/web/app/(app)/automation/ReportMistake.tsx (3)
72-74: Remove commented codeRemove the commented DialogDescription to improve code clarity. If the description is needed in the future, track it in a TODO comment or version control instead.
- {/* <DialogDescription> - Explain what went wrong and our AI will suggest a fix. - </DialogDescription> */}
106-119: Improve error handling in test rerunThe error handling could be enhanced by clearing the previous test result on error and providing more specific error information.
const result = await testAiAction({ messageId: message.id, threadId: message.threadId, }); if (isActionError(result)) { + setTestResult(undefined); toastError({ title: "There was an error testing the email", - description: result.error, + description: `Error: ${result.error}. Please try again or contact support if the issue persists.`, }); } else { setTestResult(result); }
334-347: Enhance error handling and state managementThe error handling should be more robust with proper cleanup in all cases and more informative error messages.
onClick={async () => { setIsSaving(true); - const res = await updateRuleInstructionsAction({ - id: ruleId, - instructions: fixedInstructions, - }); + try { + const res = await updateRuleInstructionsAction({ + id: ruleId, + instructions: fixedInstructions, + }); - if (isActionError(res)) { - toastError({ description: res.error }); - } else { - toastSuccess({ description: "Rule updated!" }); - } - setIsSaving(false); + if (isActionError(res)) { + toastError({ + title: "Failed to update rule", + description: `Error: ${res.error}. Please try again.` + }); + } else { + toastSuccess({ description: "Rule updated successfully!" }); + } + } catch (error) { + toastError({ + title: "Unexpected error", + description: "Failed to update the rule. Please try again." + }); + } finally { + setIsSaving(false); + } }}
| <div> | ||
| <Label name="matchedRule" label="Matched rule:" /> | ||
| <div className="mt-1"> | ||
| <TestResultDisplay result={result!} /> |
There was a problem hiding this comment.
Add null check for test result access
The code assumes result is non-null with the ! operator, which could cause runtime errors. Add proper null checking.
- <TestResultDisplay result={result!} />
+ {result ? (
+ <TestResultDisplay result={result} />
+ ) : (
+ <div>No test result available</div>
+ )}📝 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.
| <TestResultDisplay result={result!} /> | |
| {result ? ( | |
| <TestResultDisplay result={result} /> | |
| ) : ( | |
| <div>No test result available</div> | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/web/utils/actions/ai-rule.ts (1)
116-118: Consider enhancing error message handling.The error message from
safeParsemight be too technical for end users. Consider mapping validation errors to more user-friendly messages.- if (!success) return { error: error.message }; + if (!success) { + const errorMessage = error.errors.map(err => + `Invalid ${err.path.join('.')}: ${err.message}` + ).join('. '); + return { error: `Invalid input: ${errorMessage}` }; + }apps/web/utils/gmail/thread.ts (1)
85-87: Consider adding type narrowing for better type safetyThe implementation is clean and follows good practices. However, we can improve type safety.
Consider this TypeScript improvement:
-export function hasMultipleMessages(thread: gmail_v1.Schema$Thread) { +export function hasMultipleMessages(thread: gmail_v1.Schema$Thread): boolean { return !!thread.messages && thread.messages.length > 1; }apps/web/app/api/google/webhook/process-history.ts (1)
Line range hint
355-424: Consider implementing retry mechanism for transient failuresWhile the error handling is good for known cases, the webhook processing could benefit from a more robust retry mechanism for transient failures, especially given the async nature of Gmail API operations.
Consider implementing:
- Exponential backoff for retries
- Dead letter queue for failed operations
- Circuit breaker pattern for API calls
This would improve reliability, especially for premium users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/api/google/webhook/process-history.ts(2 hunks)apps/web/utils/actions/ai-rule.ts(7 hunks)apps/web/utils/gmail/thread.ts(1 hunks)
🔇 Additional comments (6)
apps/web/utils/actions/ai-rule.ts (3)
876-945: LGTM! Well-structured implementation with comprehensive error handling.
The function properly validates inputs, handles errors, and coordinates between different services.
878-881: 🛠️ Refactor suggestion
Add rate limiting to prevent potential abuse.
Consider implementing rate limiting to protect the AI service from excessive requests.
async (unsafeBody: ReportAiMistakeBody) => {
const session = await auth();
if (!session?.user.id) return { error: "Not logged in" };
+ const rateLimitKey = `report-mistake:${session.user.id}`;
+ const rateLimit = await getRateLimit(rateLimitKey);
+ if (!rateLimit.success) {
+ return { error: "Too many requests. Please try again later." };
+ }Likely invalid or redundant comment.
889-910: 🛠️ Refactor suggestion
Consider wrapping database operations in a transaction.
The multiple database operations should be wrapped in a transaction to ensure data consistency.
- const [correctRule, incorrectRule, user] = await Promise.all([
+ const result = await prisma.$transaction(async (tx) => {
+ const [correctRule, incorrectRule, user] = await Promise.all([
correctRuleId
? prisma.rule.findUnique({
where: { id: correctRuleId, userId: session.user.id },
})
: null,
incorrectRuleId
? prisma.rule.findUnique({
where: { id: incorrectRuleId, userId: session.user.id },
})
: null,
prisma.user.findUnique({
where: { id: session.user.id },
select: {
email: true,
about: true,
aiProvider: true,
aiModel: true,
aiApiKey: true,
},
}),
- ]);
+ ]);
+ return { correctRule, incorrectRule, user };
+ });
+ const { correctRule, incorrectRule, user } = result;Likely invalid or redundant comment.
apps/web/app/api/google/webhook/process-history.ts (3)
17-17: LGTM! Clean import addition
The import of hasMultipleMessages is properly organized alongside related functions.
Line range hint 417-424: LGTM! Improved error handling for Gmail API quirks
The error handling is well-documented and properly handles the known Gmail API limitation for snoozed emails. The logging is informative and will help with debugging.
355-355: Verify cold email blocker behavior remains consistent
The switch to hasMultipleMessages is good for maintainability, but we should verify it maintains the same behavior as the previous implementation.
Let's verify the change doesn't affect existing threads:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the cold email blocker behavior:
Cold email blocker behavior remains consistent with hasMultipleMessages implementation
The implementation of hasMultipleMessages in apps/web/utils/gmail/thread.ts directly checks for thread.messages.length > 1, which maintains the same thread validation logic. The cold email blocker specifically requires !isThread (single message) to run, as seen in shouldRunColdEmailBlocker function in process-history.ts:
return (
coldEmailBlocker &&
coldEmailBlocker !== ColdEmailSetting.DISABLED &&
hasColdEmailAccess &&
!isThread
);This ensures cold email blocking only triggers on initial (non-thread) messages, maintaining consistent behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing cold email blocker implementations to ensure consistent behavior
rg -A 5 "coldEmailBlocker.*thread" --type typescript
Length of output: 89
Script:
#!/bin/bash
# Search for hasMultipleMessages implementation and cold email blocker usage
rg -A 5 "hasMultipleMessages|coldEmailBlocker"
Length of output: 21202
Script:
#!/bin/bash
# Find the previous implementation using git
git show HEAD~1 -- "apps/web/app/api/google/webhook/process-history.ts"
Length of output: 71
Script:
#!/bin/bash
# Search for thread-related validations
ast-grep --pattern 'function hasMultipleMessages($_) { $$$ }'
Length of output: 295
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/web/app/(app)/automation/ReportMistake.tsx (2)
319-322: Enhance error messages with more specific informationThe current error messages could be more helpful by including specific details about what went wrong.
- toastError({ - title: "Error reporting mistake", - description: response.error, - }); + toastError({ + title: "Error reporting mistake", + description: `Failed to report mistake: ${response.error}. Please ensure all required fields are filled correctly and try again.`, + });
40-404: Consider architectural improvements for state management and race conditions
State Management: As the component grows more complex, consider using a state management solution like Zustand or Jotai for better state organization and maintenance.
Race Conditions: The form submissions could potentially race if a user submits multiple times quickly. Consider implementing a debounce mechanism or disabling the submit button during submission.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)
🔇 Additional comments (3)
apps/web/app/(app)/automation/ReportMistake.tsx (3)
61-61: Replace invalid 'size-4' class with 'h-4 w-4'
The class 'size-4' is not a valid Tailwind CSS utility.
238-240: Remove debug console.error statements
Also applies to: 310-312
327-333: Address TODO comment about undefined ruleId
| // shouldn't happen | ||
| <p>No rule matched</p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for the "no rule matched" case
Instead of a comment stating "shouldn't happen", implement proper error handling and logging for this edge case.
- // shouldn't happen
- <p>No rule matched</p>
+ <div className="text-red-500">
+ <p>No rule matched. This is unexpected.</p>
+ <p>Please report this issue to the development team.</p>
+ </div>📝 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.
| // shouldn't happen | |
| <p>No rule matched</p> | |
| <div className="text-red-500"> | |
| <p>No rule matched. This is unexpected.</p> | |
| <p>Please report this issue to the development team.</p> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
apps/web/app/(app)/automation/ReportMistake.tsx (3)
76-78: Remove commented out DialogDescriptionCommented out code should be either removed or implemented. If this description is needed, implement it; otherwise, remove it.
Apply this diff:
- {/* <DialogDescription> - Explain what went wrong and our AI will suggest a fix. - </DialogDescription> */}
325-325: Extract success messages to constantsSuccess messages are duplicated. Consider extracting them to constants for better maintainability.
Apply this diff:
+const SUCCESS_MESSAGES = { + RULE_UPDATED: "Rule updated!", +}; + // ... in the components - toastSuccess({ description: "Rule updated!" }); + toastSuccess({ description: SUCCESS_MESSAGES.RULE_UPDATED }); - toastSuccess({ description: "Rule updated!" }); + toastSuccess({ description: SUCCESS_MESSAGES.RULE_UPDATED });Also applies to: 468-468
81-111: Simplify complex conditional renderingThe nested conditional rendering is complex and could be simplified using early returns or component composition.
Consider extracting the conditional logic into a separate component or using early returns:
+const RuleContent = ({ correctRuleId, correctRule, ...props }) => { + if (!correctRuleId) { + return <RuleMismatch {...props} />; + } + + if (!correctRule?.runOnThreads) { + return <ImproveRulesOrShowThreadMessage {...props} correctRule={correctRule} correctRuleId={correctRuleId} />; + } + + return <ImproveRules {...props} correctRule={correctRule} correctRuleId={correctRuleId} />; +}; - {correctRuleId ? ( - correctRule?.runOnThreads ? ( - <ImproveRules - incorrectRule={incorrectRule} - correctRule={correctRule} - message={message} - result={result} - correctRuleId={correctRuleId} - setCorrectRuleId={setCorrectRuleId} - /> - ) : ( - <ImproveRulesOrShowThreadMessage - incorrectRule={incorrectRule} - correctRule={correctRule} - message={message} - result={result} - correctRuleId={correctRuleId} - setCorrectRuleId={setCorrectRuleId} - threadId={message.threadId} - /> - ) - ) : ( - <RuleMismatch - result={result} - setCorrectRuleId={setCorrectRuleId} - data={data} - isLoading={isLoading} - error={error} - message={message} - /> - )} + <RuleContent + correctRuleId={correctRuleId} + correctRule={correctRule} + incorrectRule={incorrectRule} + message={message} + result={result} + setCorrectRuleId={setCorrectRuleId} + data={data} + isLoading={isLoading} + error={error} + threadId={message.threadId} + />apps/web/app/api/google/threads/[id]/check/route.ts (2)
6-6: Consider enhancing type documentationThe
ThreadCheckResponsetype could benefit from JSDoc documentation explaining whatisThreadrepresents in the context of Gmail threads.+/** Response indicating whether a Gmail thread contains multiple messages */ export type ThreadCheckResponse = { isThread: boolean };
8-12: Consider standardizing error response structureWhile the authentication check is correct, consider standardizing the error response structure across all endpoints.
- return Response.json({ error: "Not logged in" }, { status: 401 }); + return Response.json( + { + error: { + code: "UNAUTHORIZED", + message: "Not logged in" + } + }, + { status: 401 } + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
apps/web/app/(app)/automation/ReportMistake.tsx(1 hunks)apps/web/app/(app)/automation/TestRules.tsx(7 hunks)apps/web/app/api/google/threads/[id]/check/route.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/app/(app)/automation/TestRules.tsx
🔇 Additional comments (5)
apps/web/app/(app)/automation/ReportMistake.tsx (4)
383-385: Remove debug console.error statements
Debug console statements should not be present in production code. Consider using proper error logging or monitoring service.
400-406: Address TODO comment about undefined ruleId
The TODO comment indicates uncertainty about when ruleId is undefined. This should be properly handled.
68-68: Replace invalid 'size-4' class with 'h-4 w-4'
The class 'size-4' is not a valid Tailwind CSS utility.
Also applies to: 198-198, 279-279
136-138:
Add null check for TestResultDisplay component
The current implementation assumes result is non-null when rendering TestResultDisplay. This could lead to runtime errors.
Apply this diff to add proper null checking:
- {result ? (
- <TestResultDisplay result={result} />
- ) : (
- <p>No rule matched</p>
- )}
+ {result ? (
+ result.rule ? (
+ <TestResultDisplay result={result} />
+ ) : (
+ <p>Rule evaluation completed but no rule matched</p>
+ )
+ ) : (
+ <p>No evaluation result available</p>
+ )}Likely invalid or redundant comment.
apps/web/app/api/google/threads/[id]/check/route.ts (1)
1-29: Verify integration with mistake reporting flow
Since this PR is about reporting mistakes during testing, let's verify how this thread checking endpoint is integrated into the mistake reporting flow.
✅ Verification successful
Based on the code analysis, I can now provide a final response about the thread checking endpoint's integration with the mistake reporting flow.
Thread check endpoint is correctly integrated with mistake reporting
The thread check endpoint is used in the ImproveRulesOrShowThreadMessage component to determine if a rule mismatch was due to thread settings rather than incorrect AI instructions. This integration helps users understand and fix rule application issues by:
- Checking if a message is part of a thread when a rule is configured to not run on threads
- Providing appropriate feedback to users about why a rule didn't match
- Guiding users to either fix the rule's thread settings or improve the AI instructions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of the thread check endpoint in the mistake reporting components
rg -t typescript -t tsx "threads/[^\"']+/check"
# Look for components or utilities related to mistake reporting that might use this endpoint
ast-grep --pattern 'function $reportMistake($_) {
$$$
}'
Length of output: 147
Script:
#!/bin/bash
# Search for usages of the thread check endpoint with correct file extensions
rg -t ts -t tsx "threads/.+/check"
# Look for components or utilities related to mistake reporting
rg -t ts -t tsx "reportMistake|mistake|report" -A 3
# Search for files that might be related to mistake reporting
fd -e ts -e tsx -e js -e jsx . | grep -i "mistake|report"
Length of output: 204
Script:
#!/bin/bash
# Search for usages of the thread check endpoint
rg "threads/.+/check"
# Look for components or utilities related to mistake reporting
rg "reportMistake|mistake|report"
# Find TypeScript/JavaScript files that might contain mistake reporting logic
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" -o -name "*.jsx" \) -exec grep -l -i "mistake\|report" {} \;
Length of output: 97239
Script:
#!/bin/bash
# Search for the ReportMistake component implementation
cat apps/web/app/\(app\)/automation/ReportMistake.tsx
# Look for the validation schema for mistake reporting
cat apps/web/utils/actions/validation.ts
# Look for the action implementation
cat apps/web/utils/actions/ai-rule.ts
Length of output: 44990
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/app/(app)/automation/TestRules.tsx (3)
85-127: Consider adding cleanup and error handling for test all functionalityA few suggestions to improve the robustness of the test all implementation:
- Reset
isTestingAllRefon component unmount- Add error handling for the test all loop
+ useEffect(() => { + return () => { + isTestingAllRef.current = false; + }; + }, []); const handleTestAll = async () => { handleStart(); + try { for (const message of data?.messages || []) { if (!isTestingAllRef.current) break; await onTest(message); } + } catch (error) { + toastError({ + title: "Error testing all messages", + description: error instanceof Error ? error.message : "Unknown error occurred", + }); + } finally { handleStop(); + } - handleStop(); };
Line range hint
324-324: Move MAX_LENGTH constant outside componentThe MAX_LENGTH constant should be defined outside the component to prevent recreation on each render.
+const MAX_CONTENT_LENGTH = 280; + export function TestResultDisplay({ result, prefix, }: { result: TestResult; prefix?: string; }) { if (!result) return null; if (result.actionItems) { - const MAX_LENGTH = 280;
Line range hint
287-385: Consider extracting alert components and improving type safetyThe TestResultDisplay component could be improved by:
- Extracting common Alert patterns into reusable components
- Using discriminated unions for better type safety
+type BaseAlertProps = { + title: string; + children: React.ReactNode; + variant: "destructive" | "blue"; +}; + +function RuleAlert({ title, children, variant }: BaseAlertProps) { + return ( + <Alert variant={variant} className="bg-white"> + {variant === "blue" && <CheckCircle2Icon className="h-4 w-4" />} + <AlertTitle>{title}</AlertTitle> + <AlertDescription>{children}</AlertDescription> + </Alert> + ); +} + +type TestResult = + | { rule: null; reason?: string } + | { rule: { name: string; type: RuleType; instructions: string }; actionItems?: any[]; reason?: string };This would simplify the main component and make it more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(app)/automation/TestRules.tsx(9 hunks)
🔇 Additional comments (1)
apps/web/app/(app)/automation/TestRules.tsx (1)
381-381: Replace invalid 'size-4' class with 'h-4 w-4'
The class 'size-4' is not a valid Tailwind CSS utility. Replace it with 'h-4 w-4' to properly size the icon.
-<CircleCheckIcon className="mr-2 size-4" />
+<CircleCheckIcon className="mr-2 h-4 w-4" />
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/web/app/(app)/automation/TestRules.tsx (3)
85-127: Consider simplifying state managementThe current implementation uses both
useRefanduseStateto manage testing state. This could be simplified to improve maintainability.Consider this alternative implementation:
- const isTestingAllRef = useRef(false); - const [isTestingAll, setIsTestingAll] = useState(false); + const [isTestingAll, setIsTestingAll] = useState(false); const handleTestAll = async () => { - handleStart(); + setIsTestingAll(true); for (const message of data?.messages || []) { - if (!isTestingAllRef.current) break; + if (!isTestingAll) break; await onTest(message); } - handleStop(); + setIsTestingAll(false); }; - const handleStart = () => { - setIsTestingAll(true); - isTestingAllRef.current = true; - }; - - const handleStop = () => { - isTestingAllRef.current = false; - setIsTestingAll(false); - }; + const handleStop = () => setIsTestingAll(false);
90-106: Enhance error handling for retry scenariosThe current error handling shows a toast but doesn't preserve the error state for potential retry attempts.
Consider adding error state and retry functionality:
+ const [testErrors, setTestErrors] = useState<Record<string, string>>({}); const onTest = useCallback(async (message: Message) => { setIsTesting((prev) => ({ ...prev, [message.id]: true })); + setTestErrors((prev) => ({ ...prev, [message.id]: '' })); const result = await testAiAction({ messageId: message.id, threadId: message.threadId, }); if (isActionError(result)) { + setTestErrors((prev) => ({ ...prev, [message.id]: result.error })); toastError({ title: "There was an error testing the email", description: result.error, }); } else { setTestResult((prev) => ({ ...prev, [message.id]: result })); } setIsTesting((prev) => ({ ...prev, [message.id]: false })); }, []);
Line range hint
302-389: Consider extracting HoverCard alert contentThe alert content in HoverCard could be extracted into separate components to improve maintainability and reusability.
Consider creating separate components for different alert types:
function NoRuleAlert({ result, prefix }: { result: TestResult; prefix?: string }) { return ( <Alert variant="destructive" className="bg-white"> <AlertTitle>No rule found</AlertTitle> <AlertDescription> {/* ... existing content ... */} </AlertDescription> </Alert> ); } function RuleFoundAlert({ result }: { result: TestResult }) { return ( <Alert variant="blue" className="bg-white"> <CheckCircle2Icon className="h-4 w-4" /> <AlertTitle>Rule found: "{result.rule.name}"</AlertTitle> <AlertDescription> {/* ... existing content ... */} </AlertDescription> </Alert> ); }This would simplify the TestResultDisplay component and make the alert content more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/(app)/automation/TestRules.tsx(8 hunks)
🔇 Additional comments (3)
apps/web/app/(app)/automation/TestRules.tsx (3)
42-42: LGTM: Type definition enhances code clarity
The Message type alias improves type safety and readability by providing a clear interface for message objects.
271-278: Handle undefined testResult case
The current implementation still assumes testResult is defined when showing the result.
385-385: Replace invalid 'size-4' class with 'h-4 w-4'
elie222
changed the title
1 participant
Summary by CodeRabbit
Release Notes
New Features
ReportMistakecomponent for users to report AI-generated response errors.TestRulescomponent.getMessagesfunction to increase the maximum results returned and improve authentication checks.HoverCardcomponent to allow custom class names for better styling flexibility.Bug Fixes
aiRuleFixfunction to ensure proper handling of specific email types.Chores