From e67f5c4353948d505a6b9ba727dc273306ed1035 Mon Sep 17 00:00:00 2001 From: Eliezer Steinbock <3090527+elie222@users.noreply.github.com> Date: Sun, 22 Dec 2024 21:45:20 +0200 Subject: [PATCH] Simplify ai fix rule --- apps/web/__tests__/ai-rule-fix.test.ts | 26 +- .../app/(app)/automation/ReportMistake.tsx | 496 ++++++++++++------ apps/web/app/(app)/automation/TestRules.tsx | 144 +++-- apps/web/utils/actions/ai-rule.ts | 27 +- apps/web/utils/actions/validation.ts | 15 +- apps/web/utils/ai/rule/rule-fix.ts | 87 ++- 6 files changed, 482 insertions(+), 313 deletions(-) diff --git a/apps/web/__tests__/ai-rule-fix.test.ts b/apps/web/__tests__/ai-rule-fix.test.ts index fd0a241a..3ffe4ec4 100644 --- a/apps/web/__tests__/ai-rule-fix.test.ts +++ b/apps/web/__tests__/ai-rule-fix.test.ts @@ -32,8 +32,8 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); const result = await aiRuleFix({ - incorrectRule: rule, - correctRule: null, + actualRule: rule, + expectedRule: null, email: salesEmail, user: getUser(), }); @@ -41,7 +41,7 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { console.log(result); expect(result).toBeDefined(); - expect(result.rule).toBe("matched_rule"); + expect(result.rule).toBe("actual_rule"); expect(result.fixedInstructions).toContain("sales"); expect(result.fixedInstructions).not.toBe(rule.instructions); // The new instructions should be more specific to exclude sales pitches @@ -51,10 +51,10 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); test("should fix rules when email matched wrong rule instead of correct rule", async () => { - const incorrectRule = { + const actualRule = { instructions: "Match emails about technical support and bug reports", }; - const correctRule = { + const expectedRule = { instructions: "Match emails about product feedback and feature requests", }; @@ -72,8 +72,8 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); const result = await aiRuleFix({ - incorrectRule, - correctRule, + actualRule, + expectedRule, email: feedbackEmail, user: getUser(), }); @@ -90,7 +90,7 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); test("should fix rule when email matched but shouldn't match any rules", async () => { - const incorrectRule = { + const actualRule = { instructions: "Match emails about marketing collaborations", }; @@ -108,8 +108,8 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); const result = await aiRuleFix({ - incorrectRule, - correctRule: null, + actualRule, + expectedRule: null, email: newsletterEmail, user: getUser(), }); @@ -117,7 +117,7 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { console.log(result); expect(result).toBeDefined(); - expect(result.rule).toBe("matched_rule"); + expect(result.rule).toBe("actual_rule"); expect(result.fixedInstructions).toContain("collaboration"); // The fixed rule should exclude newsletters and automated updates expect(result.fixedInstructions.toLowerCase()).toMatch( @@ -144,8 +144,8 @@ describe.skipIf(!isAiTest)("aiRuleFix", () => { }); const result = await aiRuleFix({ - incorrectRule: null, - correctRule, + actualRule: null, + expectedRule: correctRule, email: priceRequestEmail, user: getUser(), }); diff --git a/apps/web/app/(app)/automation/ReportMistake.tsx b/apps/web/app/(app)/automation/ReportMistake.tsx index 3081cfbe..09f75046 100644 --- a/apps/web/app/(app)/automation/ReportMistake.tsx +++ b/apps/web/app/(app)/automation/ReportMistake.tsx @@ -4,9 +4,11 @@ import { useCallback, useMemo, useState } from "react"; import Link from "next/link"; import { ArrowLeftIcon, + CheckIcon, ExternalLinkIcon, HammerIcon, SparklesIcon, + XIcon, } from "lucide-react"; import useSWR from "swr"; import { type SubmitHandler, useForm } from "react-hook-form"; @@ -42,6 +44,9 @@ import { Badge } from "@/components/Badge"; import { TestResultDisplay } from "@/app/(app)/automation/TestRules"; import { isReplyInThread } from "@/utils/thread"; import { isAIRule } from "@/utils/condition"; +import { Loading } from "@/components/Loading"; + +type ReportMistakeView = "select-expected-rule" | "ai-fix" | "manual-fix"; const NONE_RULE_ID = "__NONE__"; @@ -55,12 +60,7 @@ export function ReportMistake({ const { data, isLoading, error } = useSWR( "/api/user/rules", ); - const [correctRuleId, setCorrectRuleId] = useState(null); - const incorrectRule = result?.rule; - const correctRule = useMemo( - () => data?.find((rule) => rule.id === correctRuleId), - [data, correctRuleId], - ); + const actualRule = result?.rule; return ( @@ -74,58 +74,227 @@ export function ReportMistake({ Improve Rules - {/* - Explain what went wrong and our AI will suggest a fix. - */} - {correctRuleId ? ( - correctRule?.runOnThreads ? ( - + {data && ( + - ) : ( - - ) - ) : ( - - )} + )} + ); } +function Content({ + rules, + message, + result, + actualRule, +}: { + rules: RulesResponse; + message: MessagesResponse["messages"][number]; + result: TestResult | null; + actualRule: Rule | null; +}) { + const [loadingAiFix, setLoadingAiFix] = useState(false); + const [fixedInstructions, setFixedInstructions] = useState<{ + ruleId: string; + fixedInstructions: string; + }>(); + const fixedInstructionsRule = useMemo( + () => rules.find((rule) => rule.id === fixedInstructions?.ruleId), + [rules, fixedInstructions], + ); + + const [expectedRuleId, setExpectedRuleId] = useState(null); + const expectedRule = useMemo( + () => rules.find((rule) => rule.id === expectedRuleId), + [rules, expectedRuleId], + ); + + const [view, setView] = useState("select-expected-rule"); + const [_viewStack, setViewStack] = useState([ + "select-expected-rule", + ]); + + const onSetView = useCallback((newView: ReportMistakeView) => { + setViewStack((stack) => [...stack, newView]); + setView(newView); + }, []); + + const onBack = useCallback(() => { + setViewStack((stack) => { + if (stack.length <= 1) return stack; + const newStack = stack.slice(0, -1); + setView(newStack[newStack.length - 1]); + return newStack; + }); + }, []); + + const onSelectExpectedRule = useCallback( + async (expectedRuleId: string | null) => { + setExpectedRuleId(expectedRuleId); + + // if AI rule, then use AI to suggest a fix + if (expectedRule && isAIRule(expectedRule)) { + onSetView("ai-fix"); + setLoadingAiFix(true); + const response = await reportAiMistakeAction({ + actualRuleId: result?.rule?.id, + expectedRuleId, + email: { + from: message.headers.from, + subject: message.headers.subject, + snippet: message.snippet, + textHtml: message.textHtml || null, + textPlain: message.textPlain || null, + }, + }); + + setLoadingAiFix(false); + if (isActionError(response)) { + toastError({ + title: "Error reporting mistake", + description: response.error, + }); + } else { + if (response.ruleId) { + setFixedInstructions({ + ruleId: response.ruleId, + fixedInstructions: response.fixedInstructions, + }); + } else { + toastError({ + title: "Error reporting mistake", + description: + "No rule ID returned. Please contact support if this persists.", + }); + } + } + } else { + // if not AI rule, then show the manual fix view + onSetView("manual-fix"); + } + }, + [message, result?.rule?.id, onSetView, expectedRule], + ); + + if ( + expectedRule?.runOnThreads && + isReplyInThread(message.id, message.threadId) + ) { + return ( + + ); + } + + switch (view) { + case "select-expected-rule": + return ( + + ); + case "ai-fix": + return ( + onSetView("manual-fix")} + /> + ); + case "manual-fix": + return ( + + ); + default: + // biome-ignore lint/correctness/noSwitchDeclarations: intentional exhaustive check + const exhaustiveCheck: never = view; + return exhaustiveCheck; + } +} + +function AIFixView({ + loadingAiFix, + fixedInstructions, + fixedInstructionsRule, + onBack, + onReject, +}: { + loadingAiFix: boolean; + fixedInstructions: { + ruleId: string; + fixedInstructions: string; + } | null; + fixedInstructionsRule: Rule | null; + onBack: () => void; + onReject: () => void; +}) { + if (loadingAiFix) { + return ( +
+ Suggesting a fix... + +
+ ); + } + + return ( +
+ {fixedInstructions?.fixedInstructions ? ( +
+ + +
+ ) : ( +

No rule found for the fixed instructions.

+ )} + + {fixedInstructions?.ruleId && ( + + )} + +
+ ); +} + function RuleMismatch({ result, - setCorrectRuleId, - data, - isLoading, - error, + rules, + onSelectExpectedRuleId, }: { result: TestResult | null; - setCorrectRuleId: (ruleId: string | null) => void; - data?: RulesResponse; - isLoading: boolean; - error?: { error: string }; + rules: RulesResponse; + onSelectExpectedRuleId: (ruleId: string | null) => void; }) { return (
@@ -140,84 +309,65 @@ function RuleMismatch({
- -
- {[{ id: NONE_RULE_ID, name: "None" }, ...(data || [])] - .filter((rule) => rule.id !== (result?.rule?.id || NONE_RULE_ID)) - .map((rule) => ( - - ))} -
- +
+ {[{ id: NONE_RULE_ID, name: "None" }, ...rules] + .filter((rule) => rule.id !== (result?.rule?.id || NONE_RULE_ID)) + .map((rule) => ( + + ))} +
); } -interface ImproveRulesProps { - incorrectRule?: Rule | null; - correctRule?: Rule | null; - message: MessagesResponse["messages"][number]; - result: TestResult | null; - correctRuleId: string | null; - setCorrectRuleId: (ruleId: string | null) => void; -} - -/** - * If the rule is set to only run on threads, then check if this message is part of a thread. - * If it is, then that's the reason we had a mismatch, and not because of the AI instructions. - */ -function ImproveRulesOrShowThreadMessage({ - threadId, - ...props -}: ImproveRulesProps & { threadId: string }) { - const isThread = isReplyInThread(props.message.id, threadId); - - if (isThread) { - return ( -
- - This rule didn't match because the message is part of a thread, but - this rule is set to not run on threads. - -
- - -
+function ThreadSettingsMismatchMessage({ + expectedRuleId, + onBack, +}: { + expectedRuleId: string; + onBack: () => void; +}) { + return ( +
+ + This rule didn't match because the message is part of a thread, but this + rule is set to not run on threads. + +
+ +
- ); - } - - return ; +
+ ); } -function ImproveRules({ - incorrectRule, - correctRule, +function ManualFixView({ + actualRule, + expectedRule, message, result, - correctRuleId, - setCorrectRuleId, -}: ImproveRulesProps) { + onBack, +}: { + actualRule?: Rule | null; + expectedRule?: Rule | null; + message: MessagesResponse["messages"][number]; + result: TestResult | null; + onBack: () => void; +}) { const [checking, setChecking] = useState(false); const [testResult, setTestResult] = useState(); @@ -225,18 +375,15 @@ function ImproveRules({ <> <> {result && } - {incorrectRule && ( + {actualRule && ( <> - {isAIRule(incorrectRule) ? ( - + {isAIRule(actualRule) ? ( + ) : (
-

- {incorrectRule.name} is not an AI rule. -

@@ -370,11 +522,11 @@ function RuleForm({ function AIFixForm({ message, result, - correctRuleId, + expectedRuleId, }: { message: MessagesResponse["messages"][number]; result: TestResult | null; - correctRuleId: string | null; + expectedRuleId: string | null; }) { const [fixedInstructions, setFixedInstructions] = useState<{ ruleId: string; @@ -388,8 +540,9 @@ function AIFixForm({ } = useForm({ resolver: zodResolver(reportAiMistakeBody), defaultValues: { - incorrectRuleId: result?.rule?.id, - correctRuleId: correctRuleId === NONE_RULE_ID ? undefined : correctRuleId, + actualRuleId: result?.rule?.id, + expectedRuleId: + expectedRuleId === NONE_RULE_ID ? undefined : expectedRuleId, email: { from: message.headers.from, subject: message.headers.subject, @@ -457,6 +610,7 @@ function AIFixForm({ setFixedInstructions(undefined)} /> )}
@@ -466,38 +620,60 @@ function AIFixForm({ function SuggestedFix({ ruleId, fixedInstructions, + onReject, }: { ruleId: string; fixedInstructions: string; + onReject: () => void; }) { const [isSaving, setIsSaving] = useState(false); return (
-

Suggested fix:

-
- {fixedInstructions} + +
+ +
-
+ ); +} - if (isActionError(res)) { - toastError({ description: res.error }); - } else { - toastSuccess({ description: "Rule updated!" }); - } - setIsSaving(false); - }} - > - Save - +function Instructions({ + label, + instructions, +}: { + label: string; + instructions: string; +}) { + return ( +
+

{label}

+
+ {instructions} +
); } diff --git a/apps/web/app/(app)/automation/TestRules.tsx b/apps/web/app/(app)/automation/TestRules.tsx index dfbb9985..f3b8210e 100644 --- a/apps/web/app/(app)/automation/TestRules.tsx +++ b/apps/web/app/(app)/automation/TestRules.tsx @@ -335,80 +335,78 @@ export function TestResultDisplay({ ); } - if (result.actionItems) { - const MAX_LENGTH = 280; + const MAX_LENGTH = 280; - const aiGeneratedContent = result.actionItems.map((action, i) => ( -
-
- {capitalCase(action.type)} -
- {Object.entries(action) - .filter( - ([key, value]) => - value && - ["label", "subject", "content", "to", "cc", "bcc"].includes(key), - ) - .map(([key, value]) => ( -
- - {capitalCase(key)}: - - - {value} - -
- ))} + const aiGeneratedContent = result.actionItems?.map((action, i) => ( +
+
+ {capitalCase(action.type)}
- )); + {Object.entries(action) + .filter( + ([key, value]) => + value && + ["label", "subject", "content", "to", "cc", "bcc"].includes(key), + ) + .map(([key, value]) => ( +
+ + {capitalCase(key)}: + + + {value} + +
+ ))} +
+ )); - return ( - - - - Matched rule "{result.rule.name}" - - View rule - - - - - {isAIRule(result.rule) && ( -
- AI Instructions: - {result.rule.instructions.substring(0, MAX_LENGTH)} - {result.rule.instructions.length >= MAX_LENGTH && "..."} -
- )} - {!!aiGeneratedContent.length && ( -
{aiGeneratedContent}
- )} - {!!result.reason && ( -
- AI Reasoning: - {result.reason} -
- )} - - - } - > - - {prefix ? prefix : ""} - {result.rule.name} - - - - ); - } + return ( + + + + Matched rule "{result.rule.name}" + + View rule + + + + + {isAIRule(result.rule) && ( +
+ AI Instructions: + {result.rule.instructions.substring(0, MAX_LENGTH)} + {result.rule.instructions.length >= MAX_LENGTH && "..."} +
+ )} + {!!aiGeneratedContent?.length && ( +
{aiGeneratedContent}
+ )} + {!!result.reason && ( +
+ AI Reasoning: + {result.reason} +
+ )} + + + } + > + + {prefix ? prefix : ""} + {result.rule.name} + + + + ); } diff --git a/apps/web/utils/actions/ai-rule.ts b/apps/web/utils/actions/ai-rule.ts index d0b8b6b7..44334bc5 100644 --- a/apps/web/utils/actions/ai-rule.ts +++ b/apps/web/utils/actions/ai-rule.ts @@ -870,20 +870,20 @@ export const reportAiMistakeAction = withActionInstrumentation( const { success, data, error } = reportAiMistakeBody.safeParse(unsafeBody); if (!success) return { error: error.message }; - const { correctRuleId, incorrectRuleId, email, explanation } = data; + const { expectedRuleId, actualRuleId, email, explanation } = data; - if (!correctRuleId && !incorrectRuleId) + if (!expectedRuleId && !actualRuleId) return { error: "Either correct or incorrect rule ID is required" }; - const [correctRule, incorrectRule, user] = await Promise.all([ - correctRuleId + const [expectedRule, actualRule, user] = await Promise.all([ + expectedRuleId ? prisma.rule.findUnique({ - where: { id: correctRuleId, userId: session.user.id }, + where: { id: expectedRuleId, userId: session.user.id }, }) : null, - incorrectRuleId + actualRuleId ? prisma.rule.findUnique({ - where: { id: incorrectRuleId, userId: session.user.id }, + where: { id: actualRuleId, userId: session.user.id }, }) : null, prisma.user.findUnique({ @@ -898,11 +898,10 @@ export const reportAiMistakeAction = withActionInstrumentation( }), ]); - if (correctRuleId && !correctRule) - return { error: "Correct rule not found" }; + if (expectedRuleId && !expectedRule) + return { error: "Expected rule not found" }; - if (incorrectRuleId && !incorrectRule) - return { error: "Incorrect rule not found" }; + if (actualRuleId && !actualRule) return { error: "Actual rule not found" }; if (!user) return { error: "User not found" }; @@ -914,8 +913,8 @@ export const reportAiMistakeAction = withActionInstrumentation( const result = await aiRuleFix({ user, - incorrectRule, - correctRule, + actualRule, + expectedRule, email: { ...email, content, @@ -927,7 +926,7 @@ export const reportAiMistakeAction = withActionInstrumentation( if (!result) return { error: "Error fixing rule" }; return { - ruleId: result.rule === "matched_rule" ? incorrectRuleId : correctRuleId, + ruleId: result.rule === "actual_rule" ? actualRuleId : expectedRuleId, fixedInstructions: result.fixedInstructions, }; }, diff --git a/apps/web/utils/actions/validation.ts b/apps/web/utils/actions/validation.ts index 1acd45cc..23d51093 100644 --- a/apps/web/utils/actions/validation.ts +++ b/apps/web/utils/actions/validation.ts @@ -144,17 +144,14 @@ export const reportAiMistakeBody = z textHtml: z.string().nullish(), textPlain: z.string().nullish(), }), - correctRuleId: z.string().nullish(), - incorrectRuleId: z.string().nullish(), + actualRuleId: z.string().nullish(), + expectedRuleId: z.string().nullish(), explanation: z.string().nullish(), }) - .refine( - (data) => data.correctRuleId != null || data.incorrectRuleId != null, - { - message: "Either correctRuleId or incorrectRuleId must be provided", - path: ["correctRuleId"], // This will show the error on the correctRuleId field - }, - ); + .refine((data) => data.actualRuleId != null || data.expectedRuleId != null, { + message: "Either the actual or the expected rule must be provided", + path: ["expectedRuleId"], // This will show the error on the expectedRuleId field + }); export type ReportAiMistakeBody = z.infer; // categories diff --git a/apps/web/utils/ai/rule/rule-fix.ts b/apps/web/utils/ai/rule/rule-fix.ts index 178e6296..f3522291 100644 --- a/apps/web/utils/ai/rule/rule-fix.ts +++ b/apps/web/utils/ai/rule/rule-fix.ts @@ -7,31 +7,30 @@ import { chatCompletionObject } from "@/utils/llms"; import type { UserAIFields } from "@/utils/llms/types"; import type { Rule, User } from "@prisma/client"; import { createScopedLogger } from "@/utils/logger"; -import stripIndent from "strip-indent"; const logger = createScopedLogger("AI Rule Fix"); export type RuleFixResponse = { - rule: "matched_rule" | "correct_rule"; + rule: "actual_rule" | "expected_rule"; fixedInstructions: string; }; export async function aiRuleFix({ user, - incorrectRule, - correctRule, + actualRule, + expectedRule, email, explanation, }: { user: Pick & UserAIFields; - incorrectRule: Pick | null; - correctRule: Pick | null; + actualRule: Pick | null; + expectedRule: Pick | null; email: EmailForLLM; explanation?: string; }): Promise { const { problem, schema, examples } = getRuleFixPromptConfig( - incorrectRule, - correctRule, + actualRule, + expectedRule, ); const system = `You are an AI assistant that helps fix and improve email rules. @@ -78,15 +77,14 @@ Please provide the fixed rule.`; }); const res = aiResponse.object as { - rule?: "matched_rule" | "correct_rule"; + rule?: "actual_rule" | "expected_rule"; fixedInstructions: string; }; logger.trace(res); return { - rule: - res.rule ?? (incorrectRule === null ? "correct_rule" : "matched_rule"), + rule: res.rule ?? (actualRule === null ? "expected_rule" : "actual_rule"), fixedInstructions: res.fixedInstructions, }; } @@ -102,63 +100,64 @@ function getRuleFixPromptConfig( } { if (incorrectRule && correctRule) { return { - problem: stripIndent(`Here is the rule it matched against: - - ${incorrectRule.instructions} - - - Here is the rule it should have matched: - - ${correctRule.instructions} - `), + problem: `Here is the rule it matched against: + +${incorrectRule.instructions} + + +Here is the rule it was expected to match: + +${correctRule.instructions} + + +Based on the email content, determine which rule needs fixing and provide its improved version. The fixed rule should correctly handle the email while maintaining the original rule's intent.`, schema: z.object({ - rule: z.enum(["matched_rule", "correct_rule"]), + rule: z.enum(["actual_rule", "expected_rule"]), fixedInstructions: z.string().describe("The updated instructions"), }), examples: [ - stripIndent(`{ - "rule": "matched_rule", - "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." - }`), - stripIndent(`{ - "rule": "correct_rule", - "fixedInstructions": "Match cold emails from recruiters about job opportunities, but exclude automated job alerts or marketing emails from job boards." - }`), + `{ + "rule": "actual_rule", + "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." +}`, + `{ + "rule": "expected_rule", + "fixedInstructions": "Match cold emails from recruiters about job opportunities, but exclude automated job alerts or marketing emails from job boards." +}`, ], }; } if (incorrectRule) { return { - problem: - stripIndent(`Here is the rule it matched against that it shouldn't have matched: - - ${incorrectRule.instructions} - `), + problem: `Here is the rule it matched against that it shouldn't have matched: + +${incorrectRule.instructions} +`, schema: z.object({ fixedInstructions: z.string().describe("The updated instructions"), }), examples: [ - stripIndent(`{ - "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." - }`), + `{ + "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." +}`, ], }; } if (correctRule) { return { - problem: stripIndent(`Here is the rule it should have matched: - - ${correctRule.instructions} - `), + problem: `Here is the rule it should have matched: + +${correctRule.instructions} +`, schema: z.object({ fixedInstructions: z.string().describe("The updated instructions"), }), examples: [ - stripIndent(`{ - "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." - }`), + `{ + "fixedInstructions": "Apply this rule to emails reporting technical issues, bugs, or website problems, but DO NOT apply this to technical newsletters." +}`, ], }; }