Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several changes across multiple files, focusing on enhancing error handling, refining API route logic, and improving user session validation. Key modifications include updating authentication checks in API snippets, introducing new components for categorizing senders, and enhancing the management of email archiving and categorization queues. Additionally, new hooks and utility functions have been added to streamline data fetching and state management, particularly for user categories and AI-related functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant API
participant Database
User->>Frontend: Request to categorize sender
Frontend->>API: Send categorize request
API->>Database: Fetch user session
Database-->>API: Return session data
API->>Database: Fetch sender data
Database-->>API: Return sender data
API->>Frontend: Return categorization result
Frontend->>User: Display categorization result
Possibly related PRs
Poem
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (96)
apps/web/app/(app)/request-access/EnableEarlyAccess.tsx (1)
3-5: 🛠️ Refactor suggestion
Add essential functionality to the button.
The component structure looks good, but it's missing critical functionality:
- Click handler to process the early access enablement
- Loading state during processing
- Error handling and user feedback
- Type safety with TypeScript
Consider implementing these improvements:
+ import { useState } from "react"; import { Button } from "@/components/Button"; + import { toast } from "sonner"; + + interface EnableEarlyAccessProps { + onSuccess?: () => void; + } + - export function EnableEarlyAccess() { + export function EnableEarlyAccess({ onSuccess }: EnableEarlyAccessProps) { + const [isLoading, setIsLoading] = useState(false); + + const handleEnableAccess = async () => { + try { + setIsLoading(true); + // TODO: Add API call to enable early access + toast.success("Early access features enabled!"); + onSuccess?.(); + } catch (error) { + toast.error("Failed to enable early access features"); + } finally { + setIsLoading(false); + } + }; + - return <Button id="beta-button">Enable Early Access Features</Button>; + return ( + <Button + id="beta-button" + onClick={handleEnableAccess} + loading={isLoading} + disabled={isLoading} + > + Enable Early Access Features + </Button> + ); }Committable suggestion skipped: line range outside the PR's diff.
apps/web/hooks/useCategories.ts (1)
4-10: 🛠️ Refactor suggestion
Consider enhancing the hook with SWR configuration options.
While the implementation is functional, it could benefit from additional configuration for better performance and reliability:
- Add revalidation options
- Include error retry configuration
- Improve type safety for the fallback array
Consider applying these improvements:
export function useCategories() { const { data, isLoading, error, mutate } = useSWR<UserCategoriesResponse>( "/api/user/categories", + { + revalidateOnFocus: false, + revalidateOnReconnect: true, + retry: 3, + dedupingInterval: 60000, // 1 minute + } ); - return { categories: data?.result || [], isLoading, error, mutate }; + return { + categories: data?.result ?? ([] as UserCategoriesResponse['result']), + isLoading, + error, + mutate + }; }This enhancement:
- Reduces unnecessary revalidation on window focus
- Adds retry logic for failed requests
- Implements proper type assertion for the fallback array
- Adds deduping to prevent excessive requests
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export function useCategories() { const { data, isLoading, error, mutate } = useSWR<UserCategoriesResponse>( "/api/user/categories", { revalidateOnFocus: false, revalidateOnReconnect: true, retry: 3, dedupingInterval: 60000, // 1 minute } ); return { categories: data?.result ?? ([] as UserCategoriesResponse['result']), isLoading, error, mutate }; }apps/web/hooks/useFeatureFlags.ts (1)
10-16: 🛠️ Refactor suggestion
Add type safety for variant keys and document possible values.
Both hooks could benefit from improved type safety and documentation of possible variant values.
+type LandingPageVariant = "control" | "variant_a" | "variant_b" | null; +type AppOnboardingVariant = "simple" | "detailed" | null; + -export function useLandingPageVariant() { +export function useLandingPageVariant(): LandingPageVariant { return useFeatureFlagVariantKey("landing-page-features"); } -export function useAppOnboardingVariant() { +export function useAppOnboardingVariant(): AppOnboardingVariant { return useFeatureFlagVariantKey("app-onboarding"); }Consider adding JSDoc comments to document the purpose and possible return values of each hook:
/** * Hook to get the current landing page variant. * @returns The variant key: 'control', 'variant_a', 'variant_b', or null if the feature flag is not set */ /** * Hook to get the current app onboarding variant. * @returns The variant key: 'simple', 'detailed', or null if the feature flag is not set */apps/web/components/TopBar.tsx (2)
3-11: 🛠️ Refactor suggestion
Consider enhancing type safety and documentation
While the type definitions are good, we can improve the component's maintainability and reusability.
Consider these enhancements:
+import { type HTMLAttributes } from "react"; + +interface TopBarProps extends HTMLAttributes<HTMLDivElement> { + /** The content to be rendered inside the top bar */ + children: React.ReactNode; + /** Whether the top bar should stick to the top of the viewport */ + sticky?: boolean; + /** Additional CSS classes to be applied to the top bar */ + className?: string; +} + -export function TopBar({ +export function TopBar({ children, sticky = false, className, + ...props -}: { - children: React.ReactNode; - sticky?: boolean; - className?: string; -}) { +}: TopBarProps) {This change:
- Extracts the interface for better reusability
- Adds JSDoc comments for better documentation
- Extends HTMLAttributes for complete div props support
📝 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.import { type HTMLAttributes } from "react"; interface TopBarProps extends HTMLAttributes<HTMLDivElement> { /** The content to be rendered inside the top bar */ children: React.ReactNode; /** Whether the top bar should stick to the top of the viewport */ sticky?: boolean; /** Additional CSS classes to be applied to the top bar */ className?: string; } export function TopBar({ children, sticky = false, className, ...props }: TopBarProps) {
12-22: 🛠️ Refactor suggestion
Consider accessibility improvements
The component implementation is clean, but we can enhance its accessibility.
Consider these improvements:
return ( <div + role="banner" + {...props} className={cn( "flex flex-col justify-between gap-1 border-b bg-white px-2 py-2 shadow sm:flex-row sm:px-4", sticky && "top-0 z-10 sm:sticky", className, )} > {children} </div> );The
role="banner"attribute helps screen readers identify this as a landmark region, typically used for headers and top bars.Committable suggestion skipped: line range outside the PR's diff.
apps/web/store/QueueInitializer.tsx (1)
7-7:
⚠️ Potential issueConsider replacing module-level variable with React state
Using a module-level variable for tracking initialization state could lead to issues with concurrent rendering and React's strict mode. This approach isn't safe for server components either.
Consider refactoring to use React's useState:
-let isInitialized = false; function useInitializeQueues() { + const [isInitialized, setIsInitialized] = useState(false); const queueState = useQueueState(); useEffect(() => { - if (!isInitialized) { - isInitialized = true; + if (!isInitialized) { + setIsInitialized(true);Committable suggestion skipped: line range outside the PR's diff.
apps/web/components/EmailCell.tsx (3)
1-9: 🛠️ Refactor suggestion
Consider extracting props interface.
While the implementation is solid, consider extracting the props interface for better reusability and maintainability.
+interface EmailCellProps { + emailAddress: string; + className?: string; +} -export const EmailCell = memo(function EmailCell({ +export const EmailCell = memo(function EmailCell({ emailAddress, className, -}: { - emailAddress: string; - className?: string; -}) +}: EmailCellProps)📝 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.import { memo } from "react"; interface EmailCellProps { emailAddress: string; className?: string; } export const EmailCell = memo(function EmailCell({ emailAddress, className, }: EmailCellProps) {
17-22: 🛠️ Refactor suggestion
Enhance accessibility and UI robustness.
The rendering structure could benefit from improved accessibility and handling of long content.
Consider these improvements:
return ( - <div className={className}> + <div className={`${className} overflow-hidden`} role="cell"> - <div>{name}</div> + <div className="truncate" title={name}>{name}</div> - <div className="text-slate-500">{email}</div> + <div className="truncate text-slate-500" title={email}>{email}</div> </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.return ( <div className={`${className} overflow-hidden`} role="cell"> <div className="truncate" title={name}>{name}</div> <div className="truncate text-slate-500" title={email}>{email}</div> </div> );
10-15: 🛠️ Refactor suggestion
Enhance email parsing robustness and efficiency.
The current implementation has several potential improvements:
- Email parsing is performed twice
- The regex pattern might miss edge cases
- No validation of email format
Consider this more robust implementation:
- const parseEmail = (name: string) => { - const match = name.match(/<(.+)>/); - return match ? match[1] : name; - }; - const name = emailAddress.split("<")[0].trim(); - const email = parseEmail(emailAddress); + const parseEmailAndName = (input: string) => { + const match = input.match(/^([^<]*?)\s*(?:<(.+?)>)?$/); + if (!match) return { name: input, email: input }; + const [, name, email] = match; + return { + name: name.trim() || email, + email: email || name.trim(), + }; + }; + const { name, email } = parseEmailAndName(emailAddress);📝 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 parseEmailAndName = (input: string) => { const match = input.match(/^([^<]*?)\s*(?:<(.+?)>)?$/); if (!match) return { name: input, email: input }; const [, name, email] = match; return { name: name.trim() || email, email: email || name.trim(), }; }; const { name, email } = parseEmailAndName(emailAddress);apps/web/app/api/user/categories/route.ts (2)
8-11: 🛠️ Refactor suggestion
Consider adding input validation and simplifying the return structure.
The function could benefit from:
- Input validation for the userId parameter
- A simpler return structure without unnecessary nesting
Consider this implementation:
async function getCategories({ userId }: { userId: string }) { + if (!userId) throw new Error("userId is required"); - const result = await getUserCategories(userId); - return { result }; + return getUserCategories(userId); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.async function getCategories({ userId }: { userId: string }) { if (!userId) throw new Error("userId is required"); return getUserCategories(userId); }
15-16: 🛠️ Refactor suggestion
Enhance error response structure.
Consider using a consistent error response structure that includes a status code and more detailed message.
- return NextResponse.json({ error: "Not authenticated" }); + return NextResponse.json( + { + error: "Authentication required", + code: "UNAUTHORIZED", + message: "You must be logged in to access categories" + }, + { status: 401 } + );📝 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.if (!session?.user.id) return NextResponse.json( { error: "Authentication required", code: "UNAUTHORIZED", message: "You must be logged in to access categories" }, { status: 401 } );apps/web/hooks/useThreads.ts (1)
21-23: 🛠️ Refactor suggestion
Consider adding error handling and data transformation
The hook could benefit from error handling and data transformation logic to provide a more robust API.
const { data, isLoading, error, mutate } = useSWR<ThreadsResponse>(url, { refreshInterval, + onError: (err) => { + console.error('Error fetching threads:', err); + }, + // Transform the response data if needed + transform: (data: ThreadsResponse) => { + return { + ...data, + threads: data.threads?.map(thread => ({ + ...thread, + // Add any computed properties here + })) + }; + } });Committable suggestion skipped: line range outside the PR's diff.
apps/web/prisma/migrations/20241023204900_category/migration.sql (2)
1-2: 🛠️ Refactor suggestion
Consider adding an index on categoryId column.
The categoryId column will likely be used in JOIN operations and WHERE clauses when filtering newsletters by category. Adding an index would improve query performance.
CREATE INDEX "Newsletter_categoryId_idx" ON "Newsletter"("categoryId");
4-14:
⚠️ Potential issueMake userId NOT NULL and consider adding soft delete.
Several concerns with the Category table design:
- userId should be NOT NULL as categories should always belong to a user
- Consider adding a soft delete column (e.g., deletedAt) for safer deletion handling
Apply these modifications:
CREATE TABLE "Category" ( "id" TEXT NOT NULL, "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, "updatedAt" TIMESTAMP(3) NOT NULL, "name" TEXT NOT NULL, "description" TEXT, - "userId" TEXT, + "userId" TEXT NOT NULL, + "deletedAt" TIMESTAMP(3), CONSTRAINT "Category_pkey" PRIMARY KEY ("id") );📝 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.-- CreateTable CREATE TABLE "Category" ( "id" TEXT NOT NULL, "createdAt" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, "updatedAt" TIMESTAMP(3) NOT NULL, "name" TEXT NOT NULL, "description" TEXT, "userId" TEXT NOT NULL, "deletedAt" TIMESTAMP(3), CONSTRAINT "Category_pkey" PRIMARY KEY ("id") );apps/web/app/(app)/smart-categories/setup/page.tsx (2)
13-13:
⚠️ Potential issueAdd error handling for category fetching.
The
getUserCategoriescall should include error handling to gracefully handle potential failures.- const categories = await getUserCategories(session.user.id); + try { + const categories = await getUserCategories(session.user.id); + } catch (error) { + console.error("Failed to fetch user categories:", error); + throw new Error("Unable to load categories. Please try again later."); + }Committable suggestion skipped: line range outside the PR's diff.
8-11: 🛠️ Refactor suggestion
Enhance authentication error handling and type safety.
The current authentication check could be improved in several ways:
- The error message could be more descriptive for debugging
- Consider using a more graceful error handling approach instead of throwing
export default async function SetupCategoriesPage() { const session = await auth(); + if (!session) { + throw new Error("Authentication required to access smart categories setup"); + } + const email = session?.user.email; - if (!email) throw new Error("Not authenticated"); + if (!email) { + throw new Error("User email not found in session"); + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export default async function SetupCategoriesPage() { const session = await auth(); if (!session) { throw new Error("Authentication required to access smart categories setup"); } const email = session?.user.email; if (!email) { throw new Error("User email not found in session"); }apps/web/store/ai-queue.ts (2)
21-27: 🛠️ Refactor suggestion
Add validation and handle non-existent IDs.
Consider adding input validation and handling cases where the ID doesn't exist in the queue:
export const removeFromAiQueueAtom = (removeId: string) => { + if (typeof removeId !== 'string' || !removeId) { + console.warn('Invalid ID provided to removeFromAiQueueAtom'); + return; + } jotaiStore.set(aiQueueAtom, (prev) => { + if (!prev.has(removeId)) { + console.warn(`ID ${removeId} not found in queue`); + return prev; + } const remainingIds = new Set(prev); remainingIds.delete(removeId); return remainingIds;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const removeFromAiQueueAtom = (removeId: string) => { if (typeof removeId !== 'string' || !removeId) { console.warn('Invalid ID provided to removeFromAiQueueAtom'); return; } jotaiStore.set(aiQueueAtom, (prev) => { if (!prev.has(removeId)) { console.warn(`ID ${removeId} not found in queue`); return prev; } const remainingIds = new Set(prev); remainingIds.delete(removeId); return remainingIds; }); };
11-19: 🛠️ Refactor suggestion
Add input validation and error handling.
While the implementation is functionally correct, consider adding input validation and error handling:
export const pushToAiQueueAtom = (pushIds: string[]) => { + if (!Array.isArray(pushIds) || pushIds.length === 0) { + console.warn('Invalid or empty pushIds array provided to pushToAiQueueAtom'); + return; + } + if (pushIds.some(id => typeof id !== 'string' || !id)) { + console.warn('Invalid ID found in pushIds array'); + return; + } jotaiStore.set(aiQueueAtom, (prev) => { const newIds = new Set(prev); for (const id of pushIds) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const pushToAiQueueAtom = (pushIds: string[]) => { if (!Array.isArray(pushIds) || pushIds.length === 0) { console.warn('Invalid or empty pushIds array provided to pushToAiQueueAtom'); return; } if (pushIds.some(id => typeof id !== 'string' || !id)) { console.warn('Invalid ID found in pushIds array'); return; } jotaiStore.set(aiQueueAtom, (prev) => { const newIds = new Set(prev); for (const id of pushIds) { newIds.add(id); } return newIds; }); };apps/web/components/Select.tsx (1)
20-26: 🛠️ Refactor suggestion
Consider simplifying the conditional rendering
While the current implementation is correct, the conditional rendering could be more concise.
Consider this more idiomatic React approach:
- {props.label ? ( - <Label - name={props.name} - label={props.label} - tooltipText={props.tooltipText} - /> - ) : null} + {props.label && ( + <Label + name={props.name} + label={props.label} + tooltipText={props.tooltipText} + /> + )}📝 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.{props.label && ( <Label name={props.name} label={props.label} tooltipText={props.tooltipText} /> )}apps/web/providers/PostHogProvider.tsx (1)
48-48: 💡 Codebase verification
The
opt_in_site_appsconfiguration appears to be prematureBased on the codebase analysis:
- There's no evidence of using any PostHog site apps or experiments currently
- The only feature flag usage is minimal (in welcome/form.tsx)
- The PostHog integration is primarily used for basic analytics: pageviews, user identification, and event tracking
Consider removing this configuration until there's a concrete need for site apps, as it unnecessarily increases the JavaScript bundle size and potential security surface area.
🔗 Analysis chain
Verify the necessity of enabling PostHog site apps.
While enabling site apps could support the smart categorization feature, please consider:
- Security: This increases the exposure surface for analytics data. Ensure this aligns with your privacy policy and data handling requirements.
- Performance: Site apps may impact page load times due to additional JavaScript execution.
Could you clarify which specific PostHog site apps/features this will enable for the smart categorization implementation?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any PostHog site apps usage or configuration in the codebase rg -l "posthog.*site.*app"Length of output: 28
Script:
#!/bin/bash # Let's try different search patterns to find PostHog related configurations and usage rg -l "posthog" --type ts --type tsx # Also search for any documentation or config files mentioning site apps rg -l "site.?apps?" --type md --type ts --type tsxLength of output: 145
Script:
#!/bin/bash # Search for PostHog related files without type filtering rg -l "posthog" # Search for site apps mentions rg -l "site.?apps?" # Look specifically at the PostHogProvider file cat apps/web/providers/PostHogProvider.tsxLength of output: 2762
Script:
#!/bin/bash # Let's check the utils/posthog.ts file for any additional PostHog configuration cat apps/web/utils/posthog.ts # Check if there are any PostHog feature flags or experiments being used rg -l "posthog.(use_feature_flag|is_feature_enabled|getFeatureFlag)" # Look for any A/B testing or experiment related code rg "experiment" apps/web/app/Length of output: 4017
apps/web/app/(app)/smart-categories/setup/SmartCategoriesOnboarding.tsx (2)
18-51: 🛠️ Refactor suggestion
Enhance accessibility and user experience.
While the dialog structure is good, there are opportunities to improve accessibility and user experience.
Consider these improvements:
- <Dialog open={isOpen} onOpenChange={setIsOpen}> + <Dialog + open={isOpen} + onOpenChange={setIsOpen} + aria-describedby="smart-categories-description" + > <DialogContent> <DialogHeader> <DialogTitle>Welcome to Smart Categories</DialogTitle> - <DialogDescription> + <DialogDescription id="smart-categories-description"> Automatically categorize who emails you for better inbox management and smarter automation. </DialogDescription> </DialogHeader> <div className="grid gap-2 sm:gap-4"> <Card className="flex items-center"> - <TagsIcon className="mr-3 h-5 w-5" /> + <TagsIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Auto-categorize who emails you </Card> <Card className="flex items-center"> - <ArchiveIcon className="mr-3 h-5 w-5" /> + <ArchiveIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Bulk archive by category </Card> <Card className="flex items-center"> - <ZapIcon className="mr-3 h-5 w-5" /> + <ZapIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Use categories to optimize AI automation </Card> </div> <div> - <Button className="w-full" onClick={onClose}> + <Button + type="button" + className="w-full" + onClick={onClose} + > Get Started </Button> </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.return ( <Dialog open={isOpen} onOpenChange={setIsOpen} aria-describedby="smart-categories-description" > <DialogContent> <DialogHeader> <DialogTitle>Welcome to Smart Categories</DialogTitle> <DialogDescription id="smart-categories-description"> Automatically categorize who emails you for better inbox management and smarter automation. </DialogDescription> </DialogHeader> <div className="grid gap-2 sm:gap-4"> <Card className="flex items-center"> <TagsIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Auto-categorize who emails you </Card> <Card className="flex items-center"> <ArchiveIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Bulk archive by category </Card> <Card className="flex items-center"> <ZapIcon className="mr-3 h-5 w-5" aria-hidden="true" /> Use categories to optimize AI automation </Card> </div> <div> <Button type="button" className="w-full" onClick={onClose} > Get Started </Button> </div> </DialogContent> </Dialog> ); }
15-17: 🛠️ Refactor suggestion
Consider adding TypeScript types and extracting string literal.
While the implementation is correct, we can improve type safety and maintainability.
Consider applying these improvements:
+const ONBOARDING_KEY = "SmartCategories" as const; + export function SmartCategoriesOnboarding() { - const { isOpen, setIsOpen, onClose } = useOnboarding("SmartCategories"); + const { isOpen, setIsOpen, onClose } = useOnboarding(ONBOARDING_KEY);📝 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 ONBOARDING_KEY = "SmartCategories" as const; export function SmartCategoriesOnboarding() { const { isOpen, setIsOpen, onClose } = useOnboarding(ONBOARDING_KEY);apps/web/app/(app)/smart-categories/CategorizeWithAiButton.tsx (2)
49-51: 🛠️ Refactor suggestion
Make error message more user-friendly.
The current error message exposes technical details to the user. Consider providing a more user-friendly message while logging the technical details.
- error: (err) => { - return `Error categorizing senders: ${err.message}`; - }, + error: (err) => { + console.error("Categorization error:", err); + return "Unable to categorize senders. Please try again later."; + },📝 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.error: (err) => { console.error("Categorization error:", err); return "Unable to categorize senders. Please try again later."; },
20-59: 🛠️ Refactor suggestion
Extract onClick handler to improve readability.
The onClick handler contains complex logic that could be moved to a separate function for better maintainability.
Consider refactoring like this:
+ const handleCategorize = async () => { + if (isCategorizing) return; + + try { + setIsCategorizing(true); + const result = await handleActionCall( + "categorizeSendersAction", + categorizeSendersAction, + ); + + if (isActionError(result)) { + throw new Error(result.error); + } + + return result; + } finally { + setIsCategorizing(false); + } + }; <Button disabled={!hasAiAccess} - onClick={async () => { - if (isCategorizing) return; - toast.promise(...) - }} + onClick={() => toast.promise(handleCategorize, { + loading: "Categorizing senders...", + success: "Senders categorized successfully!", + error: (err) => `Unable to categorize senders. Please try again later.`, + })} >📝 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.<PremiumTooltip showTooltip={!hasAiAccess} openModal={openPremiumModal}> {/* Extract onClick handler to improve readability */} const handleCategorize = async () => { if (isCategorizing) return; try { setIsCategorizing(true); const result = await handleActionCall( "categorizeSendersAction", categorizeSendersAction, ); if (isActionError(result)) { throw new Error(result.error); } return result; } finally { setIsCategorizing(false); } }; <Button type="button" loading={isCategorizing} disabled={!hasAiAccess} onClick={() => toast.promise(handleCategorize, { loading: "Categorizing senders...", success: "Senders categorized successfully!", error: (err) => `Unable to categorize senders. Please try again later.`, })} > <SparklesIcon className="mr-2 size-4" /> Categorize Senders with AI </Button> </PremiumTooltip>apps/web/components/kanban/TaskCard.tsx (2)
10-14: 🛠️ Refactor suggestion
Consider improving the Task interface
The
contentfield is typed as string but based on usage inEmailCell, it specifically represents an email address.Consider making this more type-safe:
export interface Task { id: UniqueIdentifier; columnId: string; - content: string; + content: `${string}@${string}`; // or create an EmailAddress type }Committable suggestion skipped: line range outside the PR's diff.
69-80: 🛠️ Refactor suggestion
Enhance accessibility and user experience
The card's drag handle implementation could be improved.
Consider these improvements:
- <CardHeader className="space-between relative flex flex-row items-center border-b-2 border-secondary px-3 py-3"> + <CardHeader + className="space-between relative flex flex-row items-center border-b-2 border-secondary px-3 py-3" + role="region" + aria-label={`Email from ${task.content}`} + > <EmailCell emailAddress={task.content} className="mr-auto text-sm" /> <Button variant={"ghost"} {...attributes} {...listeners} - className="-mr-2 h-auto cursor-grab p-1 text-secondary-foreground/50" + className="-mr-2 h-auto cursor-grab p-1 text-secondary-foreground/50 hover:bg-secondary/80" + aria-label={`Drag email from ${task.content}`} > - <span className="sr-only">Move email</span> + <span className="sr-only">Drag to move email from {task.content}</span> <GripVertical /> </Button>📝 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.<CardHeader className="space-between relative flex flex-row items-center border-b-2 border-secondary px-3 py-3" role="region" aria-label={`Email from ${task.content}`} > <EmailCell emailAddress={task.content} className="mr-auto text-sm" /> <Button variant={"ghost"} {...attributes} {...listeners} className="-mr-2 h-auto cursor-grab p-1 text-secondary-foreground/50 hover:bg-secondary/80" aria-label={`Drag email from ${task.content}`} > <span className="sr-only">Drag to move email from {task.content}</span> <GripVertical /> </Button> </CardHeader>apps/web/app/api/user/categorize/senders/find-senders.ts (3)
66-78: 🛠️ Refactor suggestion
Improve type safety of error checking.
The error checking logic could be made more type-safe by defining proper interfaces and using type guards.
Consider this type-safe implementation:
+interface GmailError { + errors: Array<{ + message: string; + reason: string; + }>; +} + +function isGmailError(error: unknown): error is GmailError { + return ( + typeof error === "object" && + error !== null && + "errors" in error && + Array.isArray((error as any).errors) + ); +} + function isNotFoundError(error: unknown): boolean { - return ( - typeof error === "object" && - error !== null && - "errors" in error && - Array.isArray((error as any).errors) && - (error as any).errors.some( - (e: any) => + return isGmailError(error) && error.errors.some( + (e) => e.message === "Requested entity was not found." && - e.reason === "notFound", - ) + e.reason === "notFound" ); }📝 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.interface GmailError { errors: Array<{ message: string; reason: string; }>; } function isGmailError(error: unknown): error is GmailError { return ( typeof error === "object" && error !== null && "errors" in error && Array.isArray((error as any).errors) ); } function isNotFoundError(error: unknown): boolean { return isGmailError(error) && error.errors.some( (e) => e.message === "Requested entity was not found." && e.reason === "notFound" ); }
7-35:
⚠️ Potential issueAdd error handling and input validation.
The pagination implementation looks good, but there are a few concerns:
- Missing try-catch block for error handling
- No validation for the maxPages parameter
- Potential memory issues with large datasets
Consider applying these improvements:
export async function findSendersWithPagination( gmail: gmail_v1.Gmail, accessToken: string, maxPages: number, ) { + if (maxPages <= 0) throw new Error("maxPages must be greater than 0"); + if (maxPages > 100) throw new Error("maxPages cannot exceed 100"); + const allSenders: SenderMap = new Map(); let nextPageToken: string | undefined = undefined; let currentPage = 0; while (currentPage < maxPages) { + try { const { senders, nextPageToken: newNextPageToken } = await findSenders( gmail, accessToken, nextPageToken, ); + // Consider implementing batch processing if memory becomes an issue for (const [sender, messages] of Object.entries(senders)) { const existingMessages = allSenders.get(sender) ?? []; allSenders.set(sender, [...existingMessages, ...messages]); } if (!newNextPageToken) break; nextPageToken = newNextPageToken; currentPage++; + } catch (error) { + console.error("Error fetching senders:", error); + throw error; + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export async function findSendersWithPagination( gmail: gmail_v1.Gmail, accessToken: string, maxPages: number, ) { if (maxPages <= 0) throw new Error("maxPages must be greater than 0"); if (maxPages > 100) throw new Error("maxPages cannot exceed 100"); const allSenders: SenderMap = new Map(); let nextPageToken: string | undefined = undefined; let currentPage = 0; while (currentPage < maxPages) { try { const { senders, nextPageToken: newNextPageToken } = await findSenders( gmail, accessToken, nextPageToken, ); // Consider implementing batch processing if memory becomes an issue for (const [sender, messages] of Object.entries(senders)) { const existingMessages = allSenders.get(sender) ?? []; allSenders.set(sender, [...existingMessages, ...messages]); } if (!newNextPageToken) break; nextPageToken = newNextPageToken; currentPage++; } catch (error) { console.error("Error fetching senders:", error); throw error; } } return { senders: allSenders, nextPageToken }; }
37-64: 🛠️ Refactor suggestion
Enhance sender extraction and performance.
The function works correctly but could benefit from some improvements:
Consider these enhancements:
export async function findSenders( gmail: gmail_v1.Gmail, accessToken: string, pageToken?: string, - maxResults = 50, + maxResults = 50 ) { + if (maxResults > 100) throw new Error("maxResults cannot exceed 100"); const senders: SenderMap = new Map(); + try { const { threads, nextPageToken } = await getThreadsWithNextPageToken({ q: "-in:sent", gmail, maxResults, pageToken, }); - const messageIds = threads.map((t) => t.id).filter(isDefined); + // Use Set for deduplication of message IDs + const messageIds = [...new Set(threads.map((t) => t.id).filter(isDefined))]; const messages = await getMessagesBatch(messageIds, accessToken); for (const message of messages) { const sender = message.headers.from; - if (sender) { + // More robust sender extraction + if (sender && sender.includes("@")) { const existingMessages = senders.get(sender) ?? []; senders.set(sender, [...existingMessages, message]); } } return { senders, nextPageToken }; + } catch (error) { + console.error("Error in findSenders:", error); + throw error; + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export async function findSenders( gmail: gmail_v1.Gmail, accessToken: string, pageToken?: string, maxResults = 50 ) { if (maxResults > 100) throw new Error("maxResults cannot exceed 100"); const senders: SenderMap = new Map(); try { const { threads, nextPageToken } = await getThreadsWithNextPageToken({ q: "-in:sent", gmail, maxResults, pageToken, }); // Use Set for deduplication of message IDs const messageIds = [...new Set(threads.map((t) => t.id).filter(isDefined))]; const messages = await getMessagesBatch(messageIds, accessToken); for (const message of messages) { const sender = message.headers.from; // More robust sender extraction if (sender && sender.includes("@")) { const existingMessages = senders.get(sender) ?? []; senders.set(sender, [...existingMessages, message]); } } return { senders, nextPageToken }; } catch (error) { console.error("Error in findSenders:", error); throw error; } }apps/web/app/(app)/request-access/page.tsx (2)
16-18:
⚠️ Potential issueAdd interaction handling and feedback mechanisms to the button.
The button appears to be missing crucial functionality:
- No click handler to actually enable early access features
- No loading state during activation
- No success/error feedback to users
- Missing accessibility attributes
Consider applying these improvements:
- <Button id="beta-button" size="xl"> + <Button + id="beta-button" + size="xl" + onClick={handleEnableEarlyAccess} + loading={isLoading} + aria-label="Enable early access features" + > Enable Early Access Features </Button>You'll need to implement the
handleEnableEarlyAccessfunction and manage the loading state. Consider adding a toast notification for success/error feedback.Committable suggestion skipped: line range outside the PR's diff.
9-18: 💡 Codebase verification
Smart Categories feature needs specific enablement UI
Based on the codebase analysis:
- Smart Categories is controlled by its own feature flag (
smart-categories) and has dedicated setup pages- The generic "Enable Early Access Features" page doesn't specifically handle Smart Categories enablement
- The feature has its own onboarding flow at
/smart-categories/setupRecommendations:
- Add specific messaging about Smart Categories in the early access page if it's meant to be the entry point
- Consider adding a dedicated card for Smart Categories similar to the commented-out examples for other features
- Clarify the relationship between early access enablement and the
smart-categoriesfeature flag🔗 Analysis chain
Verify the feature enablement flow for Smart Categories.
Given that this PR implements Smart Categories functionality, please verify:
- Is this the correct entry point for enabling the Smart Categories feature?
- Should there be specific messaging about Smart Categories in the description?
- Will enabling early access automatically enable Smart Categories?
Let's check for any other Smart Categories related UI components:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Smart Categories related components and text echo "Searching for Smart Categories related files..." rg -l -i "smart.?categor(y|ies)" --type=tsx --type=ts echo "Searching for other feature enablement components..." rg -l "enable.*features?" --type=tsx --type=tsLength of output: 380
Script:
#!/bin/bash # Search for Smart Categories related components and text with correct file extensions echo "Searching for Smart Categories related files..." rg -l -i "smart.?categor(y|ies)" echo "Searching for early access feature enablement..." rg -l "early.*access.*feature" echo "Checking request-access page implementation..." cat "apps/web/app/(app)/request-access/page.tsx"Length of output: 3518
Script:
#!/bin/bash # Check the feature flags implementation and smart categories setup echo "Checking feature flags implementation..." cat "apps/web/hooks/useFeatureFlags.ts" echo "Checking smart categories setup page..." cat "apps/web/app/(app)/smart-categories/setup/page.tsx" echo "Checking if there's any connection between early access and smart categories..." rg -l "useFeatureFlags.*smart"Length of output: 1728
apps/web/components/Modal.tsx (1)
69-76: 🛠️ Refactor suggestion
Consider enhancing padding and size management.
While the current implementation works, there are opportunities for improvement:
- The padding logic could be more explicit about the default case
- The size values could be extracted into constants for better maintainability
Consider applying these improvements:
+ const MODAL_SIZES = { + xl: "sm:w-full sm:max-w-xl", + "2xl": "sm:w-full sm:max-w-2xl", + "4xl": "sm:w-full sm:max-w-4xl", + "6xl": "sm:w-full sm:max-w-6xl", + } as const; className={clsx( "w-full transform rounded-2xl bg-white text-left align-middle shadow-xl transition-all", { - "p-6": padding === "sm", - "p-10": !padding, + "p-6": padding === "sm", + "p-10": padding === undefined, + "p-0": padding === "none", - "sm:w-full sm:max-w-xl": !fullWidth && (!size || size === "xl"), - "sm:w-full sm:max-w-2xl": !fullWidth && size === "2xl", - "sm:w-full sm:max-w-4xl": !fullWidth && size === "4xl", - "sm:w-full sm:max-w-6xl": !fullWidth && size === "6xl", + [MODAL_SIZES[size ?? "xl"]]: !fullWidth, "sm:w-full sm:max-w-full": fullWidth, } )}Committable suggestion skipped: line range outside the PR's diff.
apps/web/components/OnboardingModal.tsx (1)
91-115:
⚠️ Potential issueSeveral improvements needed for the useOnboarding hook.
While the hook implementation is functional, there are several areas for improvement:
- The feature parameter should be sanitized before using in localStorage key
- The useEffect dependency array is missing setIsOpen
- The return object should be memoized to prevent unnecessary rerenders
- The feature parameter lacks type safety
Here's the suggested implementation:
-export const useOnboarding = (feature: string) => { +type OnboardingFeature = 'smartCategories' | 'simpleMode'; + +export const useOnboarding = (feature: OnboardingFeature) => { + // Sanitize feature name for localStorage + const storageKey = `viewed${feature.charAt(0).toUpperCase()}${feature.slice(1)}Onboarding`; + const [isOpen, setIsOpen] = useState<boolean>(false); const [hasViewedOnboarding, setHasViewedOnboarding] = useLocalStorage( - `viewed${feature}Onboarding`, + storageKey, false, ); useEffect(() => { if (!hasViewedOnboarding) { setIsOpen(true); setHasViewedOnboarding(true); } - }, [setHasViewedOnboarding, hasViewedOnboarding]); + }, [setHasViewedOnboarding, hasViewedOnboarding, setIsOpen]); const onClose = useCallback(() => { setIsOpen(false); }, []); - return { + return useMemo(() => ({ isOpen, hasViewedOnboarding, setIsOpen, onClose, - }; + }), [isOpen, hasViewedOnboarding, setIsOpen, onClose]); };📝 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.type OnboardingFeature = 'smartCategories' | 'simpleMode'; export const useOnboarding = (feature: OnboardingFeature) => { // Sanitize feature name for localStorage const storageKey = `viewed${feature.charAt(0).toUpperCase()}${feature.slice(1)}Onboarding`; const [isOpen, setIsOpen] = useState<boolean>(false); const [hasViewedOnboarding, setHasViewedOnboarding] = useLocalStorage( storageKey, false, ); useEffect(() => { if (!hasViewedOnboarding) { setIsOpen(true); setHasViewedOnboarding(true); } }, [setHasViewedOnboarding, hasViewedOnboarding, setIsOpen]); const onClose = useCallback(() => { setIsOpen(false); }, []); return useMemo(() => ({ isOpen, hasViewedOnboarding, setIsOpen, onClose, }), [isOpen, hasViewedOnboarding, setIsOpen, onClose]); };apps/web/__tests__/ai-categorize-senders.test.ts (3)
58-66: 🛠️ Refactor suggestion
Add test cases for additional edge scenarios.
While testing empty senders list is good, consider adding tests for:
- Empty categories but non-empty senders
- Single sender/category scenarios
- Maximum input size limits
it("should handle empty categories list", async () => { const senders = ["test@example.com"]; const result = await aiCategorizeSenders({ user, senders: senders.map(sender => ({ emailAddress: sender, snippet: "" })), categories: [], }); expect(result).toEqual([]); });
68-88: 🛠️ Refactor suggestion
Make category testing more realistic.
The current test creates email addresses directly from category names, which might not reflect real-world scenarios. The AI might rely too heavily on the email address format rather than the actual content.
Consider using more realistic test data:
const testData = [ { sender: "weekly-digest@newsletter.com", snippet: "Here's your weekly roundup of top stories", expectedCategory: "newsletter" }, // Add more realistic test cases ];
15-56: 🛠️ Refactor suggestion
Enhance test coverage for edge cases and error scenarios.
The current test case focuses on happy path scenarios. Consider adding tests for:
- Error handling when AI service fails
- Impact of different snippet contents on categorization
- Invalid email formats
- Rate limiting scenarios
Also, the hardcoded category expectations might make the test brittle if the AI model's behavior changes.
Consider making the test more robust:
const result = await aiCategorizeSenders({ user, - senders: senders.map((sender) => ({ emailAddress: sender, snippet: "" })), + senders: senders.map((sender) => ({ + emailAddress: sender, + snippet: "This is a test email about product updates" + })), categories: getEnabledCategories(), }); -expect(supportResult?.category).toBe("support"); +expect(supportResult?.category).toMatch(/^(support|customer_service)$/);Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/api/google/threads/controller.ts (1)
36-47:
⚠️ Potential issueAdd input validation and documentation for the query builder.
While the function effectively consolidates query logic, it needs additional safeguards and documentation:
- The
query.qandquery.fromEmailinputs should be sanitized to prevent Gmail search injection- Consider documenting the function's role in supporting sender categorization
Apply this diff to enhance security and documentation:
+ /** + * Constructs a Gmail API query string based on search criteria. + * Supports filtering by custom query, sender email, or archive status. + * Used for both general search and smart categorization features. + */ function getQuery() { if (query.q) { - return query.q; + return encodeURIComponent(query.q.trim()); } if (query.fromEmail) { - return `from:${query.fromEmail}`; + // Escape special characters in email addresses + return `from:${encodeURIComponent(query.fromEmail.trim())}`; } if (query.type === "archive") { return `-label:${INBOX_LABEL_ID}`; } return undefined; }📝 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./** * Constructs a Gmail API query string based on search criteria. * Supports filtering by custom query, sender email, or archive status. * Used for both general search and smart categorization features. */ function getQuery() { if (query.q) { return encodeURIComponent(query.q.trim()); } if (query.fromEmail) { // Escape special characters in email addresses return `from:${encodeURIComponent(query.fromEmail.trim())}`; } if (query.type === "archive") { return `-label:${INBOX_LABEL_ID}`; } return undefined; }apps/web/components/CommandK.tsx (1)
71-71: 🛠️ Refactor suggestion
Consider optimizing navigation rendering and adding error handling.
While the implementation is functionally correct, consider these improvements:
- Memoize the navigation items mapping to prevent unnecessary re-renders
- Add error boundary for navigation state failures
Here's how you could optimize the code:
+ const memoizedNavigationItems = React.useMemo( + () => + navigation.map((option) => ( + <CommandItem + key={option.name} + onSelect={() => { + router.push(option.href); + setOpen(false); + }} + > + <option.icon className="mr-2 h-4 w-4" /> + <span>{option.name}</span> + </CommandItem> + )), + [navigation, router, setOpen] + ); // In the render method, replace the mapping with: - {navigation.map((option) => ( - <CommandItem - key={option.name} - onSelect={() => { - router.push(option.href); - setOpen(false); - }} - > - <option.icon className="mr-2 h-4 w-4" /> - <span>{option.name}</span> - </CommandItem> - ))} + {memoizedNavigationItems}And add error handling:
class NavigationErrorBoundary extends React.Component { // Add implementation for error boundary } // Wrap the navigation group with: <NavigationErrorBoundary> <CommandGroup heading="Navigation"> {memoizedNavigationItems} </CommandGroup> </NavigationErrorBoundary>Also applies to: 122-134
apps/web/app/(app)/onboarding/OnboardingBulkUnsubscriber.tsx (1)
154-154: 🛠️ Refactor suggestion
Consider integrating sender categories in the unsubscribe interface.
Since this PR implements AI-based sender categorization, consider enhancing the unsubscribe interface to show the AI-detected category for each sender. This could help users make more informed decisions about which newsletters to unsubscribe from.
Example enhancement:
<TableRow key={row.name}> <TableCell> <EmailCell emailAddress={row.name} /> + {row.category && ( + <span className="ml-2 text-xs text-gray-500"> + {row.category} + </span> + )} </TableCell>Committable suggestion skipped: line range outside the PR's diff.
apps/web/next.config.mjs (1)
160-171: 🛠️ Refactor suggestion
Consider adding more security headers for comprehensive protection.
While the current security headers are good, consider adding these additional headers for enhanced security:
source: "/(.*)", headers: [ { key: "X-Content-Type-Options", value: "nosniff", }, { key: "X-Frame-Options", value: "DENY", }, { key: "X-XSS-Protection", value: "1; mode=block", }, + { + key: "Strict-Transport-Security", + value: "max-age=31536000; includeSubDomains", + }, + { + key: "Referrer-Policy", + value: "strict-origin-when-cross-origin", + }, + { + key: "Content-Security-Policy", + value: "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';", + }, ],Note: Customize the CSP header value based on your application's specific needs for external resources, inline scripts, and styles.
📝 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.{ key: "X-Content-Type-Options", value: "nosniff", }, { key: "X-Frame-Options", value: "DENY", }, { key: "X-XSS-Protection", value: "1; mode=block", }, { key: "Strict-Transport-Security", value: "max-age=31536000; includeSubDomains", }, { key: "Referrer-Policy", value: "strict-origin-when-cross-origin", }, { key: "Content-Security-Policy", value: "default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';", },apps/web/app/(app)/smart-categories/CreateCategoryButton.tsx (1)
159-165:
⚠️ Potential issueReset form validation errors when using example categories.
The handleExampleClick function should reset any existing validation errors when populating the form with example data.
const handleExampleClick = useCallback( (category: ExampleCategory) => { setValue("name", category.name); setValue("description", category.description); + clearErrors(["name", "description"]); }, - [setValue], + [setValue, clearErrors], );📝 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 handleExampleClick = useCallback( (category: ExampleCategory) => { setValue("name", category.name); setValue("description", category.description); clearErrors(["name", "description"]); }, [setValue, clearErrors], );apps/web/app/(landing)/components/page.tsx (1)
166-181: 🛠️ Refactor suggestion
Enhance the MultiSelectFilter demo with additional states and dynamic options.
While the demo shows basic functionality, consider:
- Demonstrating loading and error states
- Using the actual category options from a shared constant to prevent inconsistencies
- Adding examples of disabled options or maximum selection limits
+ import { EMAIL_CATEGORIES } from "@/constants/categories"; <div className="mt-4"> + {/* Basic usage */} <MultiSelectFilter title="Categories" - options={[ - { label: "Receipts", value: "receipts" }, - { label: "Newsletters", value: "newsletters" }, - { label: "Updates", value: "updates" }, - { label: "Alerts", value: "alerts" }, - ]} + options={EMAIL_CATEGORIES} selectedValues={selectedValues} setSelectedValues={setSelectedValues} /> + + {/* Loading state */} + <MultiSelectFilter + title="Categories (Loading)" + options={EMAIL_CATEGORIES} + selectedValues={[]} + setSelectedValues={() => {}} + isLoading + /> + + {/* With disabled options */} + <MultiSelectFilter + title="Categories (Some Disabled)" + options={EMAIL_CATEGORIES.map((opt, i) => ({ + ...opt, + disabled: i === 0, + }))} + selectedValues={[]} + setSelectedValues={() => {}} + /> </div>Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/(app)/stats/NewsletterModal.tsx (1)
217-220:
⚠️ Potential issueFix incorrect empty state message in AllEmails component.
The empty message states "There are no unarchived emails" but this component shows all emails, not just unarchived ones. This could be confusing for users.
Update the message to be more accurate:
<AlertBasic title="No emails" - description={`There are no unarchived emails. Switch to the "All" to view all emails from this sender.`} + description="No emails found from this sender." />Also applies to: 228-234
apps/web/app/(app)/smart-categories/setup/SetUpCategories.tsx (3)
164-178:
⚠️ Potential issueAdd error handling for category creation.
The category creation process lacks error handling and could leave the component in an inconsistent state if the action fails.
onClick={async () => { setIsCreating(true); - const upsertCategories = Array.from(categories.entries()).map( - ([name, enabled]) => ({ - id: combinedCategories.find((c) => c.name === name)?.id, - name, - enabled, - }), - ); + try { + const upsertCategories = Array.from(categories.entries()).map( + ([name, enabled]) => ({ + id: combinedCategories.find((c) => c.name === name)?.id, + name, + enabled, + }), + ); - await upsertDefaultCategoriesAction(upsertCategories); - setIsCreating(false); - router.push("/smart-categories"); + await upsertDefaultCategoriesAction(upsertCategories); + router.push("/smart-categories"); + } catch (error) { + console.error("Failed to create categories:", error); + // Consider adding a toast notification here + } finally { + setIsCreating(false); + } }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.loading={isCreating} onClick={async () => { setIsCreating(true); try { const upsertCategories = Array.from(categories.entries()).map( ([name, enabled]) => ({ id: combinedCategories.find((c) => c.name === name)?.id, name, enabled, }), ); await upsertDefaultCategoriesAction(upsertCategories); router.push("/smart-categories"); } catch (error) { console.error("Failed to create categories:", error); // Consider adding a toast notification here } finally { setIsCreating(false); } }}
202-247: 🛠️ Refactor suggestion
Enhance CategoryCard component with better UX and accessibility.
Consider the following improvements:
- Add loading states for actions
- Add confirmation dialog for delete action
- Improve button labels for accessibility
function CategoryCard({ category, isEnabled, onAdd, onRemove, onEdit, }: { category: CardCategory; isEnabled: boolean; onAdd: () => void; onRemove: () => void; onEdit: () => void; }) { + const [isDeleting, setIsDeleting] = useState(false); + + const handleDelete = async () => { + if (!confirm(`Are you sure you want to remove ${category.name}?`)) return; + setIsDeleting(true); + try { + await onRemove(); + } finally { + setIsDeleting(false); + } + }; return ( <Card className={cn( "flex items-center justify-between gap-2 p-4", !isEnabled && "bg-gray-50", )} + aria-label={`Category: ${category.name}`} > {/* ... rest of the component ... */} - <Button size="iconSm" variant="ghost" onClick={onRemove}> + <Button + size="iconSm" + variant="ghost" + onClick={handleDelete} + disabled={isDeleting} + aria-label={`Remove ${category.name} category`} + > <TrashIcon className="size-4" /> - <span className="sr-only">Remove</span> + <span className="sr-only">Remove {category.name}</span> </Button> </Card> ); }Committable suggestion skipped: line range outside the PR's diff.
85-107:
⚠️ Potential issueProtect against potential race conditions in useEffect.
The category update logic in useEffect could lead to race conditions if existingCategories changes rapidly.
Consider using a cleanup function and adding a mounted flag:
useEffect(() => { + let mounted = true; setCategories((prevCategories) => { + if (!mounted) return prevCategories; const newCategories = new Map(prevCategories); // ... existing logic ... return newCategories; }); + return () => { + mounted = false; + }; }, [existingCategories]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.useEffect(() => { let mounted = true; setCategories((prevCategories) => { if (!mounted) return prevCategories; const newCategories = new Map(prevCategories); // Enable any new categories from existingCategories that aren't in the current map for (const category of existingCategories) { if (!prevCategories.has(category.name)) { newCategories.set(category.name, true); } } // Disable any categories that aren't in existingCategories if (existingCategories.length) { for (const category of prevCategories.keys()) { if (!existingCategories.some((c) => c.name === category)) { newCategories.set(category, false); } } } return newCategories; }); return () => { mounted = false; }; }, [existingCategories]);apps/web/app/(app)/simple/SimpleList.tsx (1)
107-107:
⚠️ Potential issueConsider potential race condition and callback handling.
The change from an empty callback to
undefinedis unnecessary since both achieve the same result. More importantly, there's a potential race condition between archiving emails and navigation, as the router navigation occurs regardless of whether the archiving operation succeeds.Consider these improvements:
-archiveEmails(toArchive, undefined, () => {}); +archiveEmails(toArchive) + .then(() => { + if (props.nextPageToken) { + router.push(`/simple?type=${props.type}&pageToken=${props.nextPageToken}`); + } else { + const lastCategory = simpleEmailCategoriesArray[simpleEmailCategoriesArray.length - 1][0]; + if (props.type === lastCategory) { + router.push("/simple/completed"); + } else { + const next = getNextCategory(props.type); + router.push(`/simple?type=${next}`); + } + } + }) + .catch((error) => { + // Handle archiving errors + console.error("Failed to archive emails:", error); + });This change would:
- Remove the unnecessary undefined parameter
- Ensure navigation only occurs after successful archiving
- Add proper error handling
Committable suggestion skipped: line range outside the PR's diff.
apps/web/components/SideNav.tsx (1)
89-95: 🛠️ Refactor suggestion
Consider optimizing the navigation filtering logic.
While the current implementation works, we can make it more maintainable by extracting the path constant.
Consider this optimization:
export const useNavigation = () => { const showSmartCategories = useSmartCategoriesEnabled(); + const SMART_CATEGORIES_PATH = "/smart-categories"; return navigationItems.filter((item) => - item.href === "/smart-categories" ? showSmartCategories : true, + item.href === SMART_CATEGORIES_PATH ? showSmartCategories : true, ); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const useNavigation = () => { const showSmartCategories = useSmartCategoriesEnabled(); const SMART_CATEGORIES_PATH = "/smart-categories"; return navigationItems.filter((item) => item.href === SMART_CATEGORIES_PATH ? showSmartCategories : true, ); };.vscode/typescriptreact.code-snippets (1)
53-53: 💡 Codebase verification
Security check: Inconsistent session validation patterns detected
The codebase shows inconsistent authentication checks across API routes:
- Most routes use direct
NextResponse.json({ error: "Not authenticated" })without explicit session checks- Only one route (
cold-email/route.ts) uses the!session?.userpattern- Some routes check
!useror!userEmailinstead- One route checks
!user?.emailThis suggests that the VSCode snippet's security enhancement pattern (
!session?.user) is not consistently applied in the actual codebase. The authentication checks should be standardized across all API routes for consistent security enforcement.🔗 Analysis chain
Security enhancement: Improved session validation in API route snippets.
The authentication checks have been strengthened by validating the presence of a user object within the session. This is a good security practice as it ensures not just that a session exists, but that it contains a valid authenticated user.
Let's verify that this pattern is consistently applied across all API routes in the codebase:
Also applies to: 66-66, 85-85, 225-225
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining basic session checks that might need updating # Look for potential session checks that don't verify the user object # Search for session validation patterns in API routes rg -g '*/api/**/*' 'if.*!session[^?.].*return.*Response' # Search for NextResponse usage with authentication errors to ensure consistent error handling rg -g '*/api/**/*' 'NextResponse.*json.*Not authenticated'Length of output: 417
Script:
#!/bin/bash # Let's first check if there are any API routes in the codebase fd "route\.(ts|js|tsx|jsx)$" app # Then search for session validation patterns in all files rg "if.*!session.*return.*Response" # Also search for authentication error responses rg "NextResponse.*json.*[\"']Not authenticated[\"']"Length of output: 5719
apps/web/app/(app)/smart-categories/board/page.tsx (4)
28-30:
⚠️ Potential issueInclude uncategorized senders to display all relevant data
The query filters out senders with
categoryId: null, but the mapping function handless.categoryId || "Uncategorized", suggesting that uncategorized senders should be displayed. To ensure all senders are included, update the query to include senders without a category.Apply this diff to include uncategorized senders:
prisma.newsletter.findMany({ where: { userId: session.user.id }, - categoryId: { not: null } }, select: { id: true, email: true, categoryId: true }, }),Also applies to: 53-53
23-23: 🛠️ Refactor suggestion
Consider redirecting unauthenticated users instead of throwing an error
Throwing an error when the user is not authenticated may lead to an unhandled exception and a less user-friendly experience. Consider redirecting the user to the login page or rendering a friendly message.
Apply this diff to redirect unauthenticated users:
if (!email) { - throw new Error("Not authenticated"); + redirect('/login'); }Ensure you import the
redirectfunction from Next.js:+import { redirect } from 'next/navigation';Committable suggestion skipped: line range outside the PR's diff.
35-41: 🛠️ Refactor suggestion
Optimize category ordering logic for better performance
Using
categories.findwithinCATEGORY_ORDER.mapcan be inefficient for large lists because it performs a search for each category. Consider creating a dictionary for constant-time lookups.Refactored code:
+ const categoryMap = new Map(categories.map((c) => [c.name, c])); const orderedCategories = [ - ...CATEGORY_ORDER.map((name) => - categories.find((c) => c.name === name), - ).filter(isDefined), + ...CATEGORY_ORDER.map((name) => categoryMap.get(name)).filter(isDefined), ...categories.filter((c) => !CATEGORY_ORDER.includes(c.name)), ];📝 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.// Order categories const categoryMap = new Map(categories.map((c) => [c.name, c])); const orderedCategories = [ ...CATEGORY_ORDER.map((name) => categoryMap.get(name)).filter(isDefined), ...categories.filter((c) => !CATEGORY_ORDER.includes(c.name)), ];
25-31: 🛠️ Refactor suggestion
Add error handling for data fetching
The
Promise.allmay reject if eithergetUserCategoriesor the Prisma query fails. Consider wrapping the data fetching in a try-catch block to handle potential errors gracefully.Example:
+try { const [categories, senders] = await Promise.all([ getUserCategories(session.user.id), prisma.newsletter.findMany({ where: { userId: session.user.id }, select: { id: true, email: true, categoryId: true }, }), ]); +} catch (error) { + // Handle errors appropriately + console.error(error); + return <div className="p-4">An error occurred while fetching data.</div>; +}Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/api/user/categorize/senders/uncategorized/route.ts (5)
64-68: 🛠️ Refactor suggestion
Return appropriate HTTP status codes for error responses
Providing specific HTTP status codes with error responses helps clients handle errors correctly and improves the API's clarity. Consider returning status codes like
401for authentication errors and500for server errors.Apply the following changes:
if (!user?.email) - return NextResponse.json({ error: "Not authenticated" }); + return NextResponse.json({ error: "Not authenticated" }, { status: 401 }); if (error) - return NextResponse.json({ error }); + return NextResponse.json({ error }, { status: 500 }); if (!gmail) - return NextResponse.json({ error: "Could not load Gmail" }); + return NextResponse.json({ error: "Could not load Gmail" }, { status: 500 }); if (!session?.accessToken) - return NextResponse.json({ error: "No access token" }); + return NextResponse.json({ error: "No access token" }, { status: 401 });📝 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.if (!user?.email) return NextResponse.json({ error: "Not authenticated" }, { status: 401 }); if (error) return NextResponse.json({ error }, { status: 500 }); if (!gmail) return NextResponse.json({ error: "Could not load Gmail" }, { status: 500 }); if (!session?.accessToken) return NextResponse.json({ error: "No access token" }, { status: 401 });
27-31:
⚠️ Potential issueAdd error handling for
getSenderscallThe asynchronous call to
getSendersmay throw an error, which could cause the function to fail unexpectedly. To handle potential errors gracefully, wrap the call in atry-catchblock.Apply the following changes:
while (uncategorizedSenders.length === 0) { + try { const result = await getSenders({ ownerEmail: email, limit, offset: currentOffset, }); + } catch (error) { + console.error("Error fetching senders:", error); + // Optionally, handle the error or rethrow it + throw error; + } const allSenders = result.data.map((sender) => sender.from); // ... rest of the loop ... }Committable suggestion skipped: line range outside the PR's diff.
71-72:
⚠️ Potential issueValidate the
offsetparameter to ensure it's a valid numberCurrently, the
offsetparameter is parsed without validation. If an invalid value is provided,offsetcould beNaN, potentially leading to unexpected behavior. Add validation to ensureoffsetis a non-negative integer.Apply the following changes:
const url = new URL(request.url); const offsetParam = url.searchParams.get("offset") || "0"; -const offset = Number.parseInt(offsetParam); +const offset = Number.parseInt(offsetParam, 10); +if (isNaN(offset) || offset < 0) { + return NextResponse.json({ error: "Invalid offset parameter" }, { status: 400 }); +}Committable suggestion skipped: line range outside the PR's diff.
63-68:
⚠️ Potential issueReorder error checks for proper error handling
To prevent accessing properties of
undefinedobjects, check forerrorbefore accessinguserorgmail. This ensures that if an error occurred during session retrieval, it is handled appropriately before other checks.Apply the following changes:
export const GET = withError(async (request: Request) => { const { gmail, user, error, session } = await getSessionAndGmailClient(); + if (error) return NextResponse.json({ error }, { status: 500 }); if (!user?.email) return NextResponse.json({ error: "Not authenticated" }, { status: 401 }); - if (error) return NextResponse.json({ error }); if (!gmail) return NextResponse.json({ error: "Could not load Gmail" }, { status: 500 }); if (!session?.accessToken) return NextResponse.json({ error: "No access token" }, { status: 401 });📝 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 { gmail, user, error, session } = await getSessionAndGmailClient(); if (error) return NextResponse.json({ error }, { status: 500 }); if (!user?.email) return NextResponse.json({ error: "Not authenticated" }, { status: 401 }); if (!gmail) return NextResponse.json({ error: "Could not load Gmail" }, { status: 500 }); if (!session?.accessToken) return NextResponse.json({ error: "No access token" }, { status: 401 });
26-54:
⚠️ Potential issueEnsure the while loop terminates appropriately
The current
whileloop could potentially iterate indefinitely ifuncategorizedSendersremains empty andgetSenderscontinues to return senders already categorized. To prevent excessive looping or a potential infinite loop, consider adding a maximum number of iterations or a condition to break the loop after a certain point.Apply the following changes:
let uncategorizedSenders: string[] = []; let currentOffset = offset; + const maxIterations = 10; + let iterations = 0; while (uncategorizedSenders.length === 0) { + if (iterations >= maxIterations) { + // Prevent infinite loop if no uncategorized senders are found + return { uncategorizedSenders }; + } const result = await getSenders({ ownerEmail: email, limit, offset: currentOffset, }); // ... rest of the loop ... currentOffset += limit; + iterations += 1; }Committable suggestion skipped: line range outside the PR's diff.
apps/web/components/kanban/multipleContainersKeyboardPreset.ts (1)
29-83:
⚠️ Potential issueReplace
returnstatements inside the loop withcontinueto prevent unintended function exitsThe
returnstatements within the loop (lines 30-31, 36-37, 47-48, 55-56, 64-65) cause the entirecoordinateGetterfunction to exit prematurely when certain conditions are met. If the intention is to skip the current iteration and proceed with the nextentry,continueshould be used instead ofreturn.Apply this diff to fix the issue:
@@ -30,7 +30,7 @@ if (!entry || entry?.disabled) { - return; + continue; } @@ -36,7 +36,7 @@ if (!rect) { - return; + continue; } @@ -47,7 +47,7 @@ if (active.data.current?.type !== "Column") { - return; + continue; } } @@ -55,7 +55,7 @@ if (active.data.current?.type === "Column") { - return; + continue; } @@ -64,7 +64,7 @@ if (active.data.current?.type === "Column") { - return; + continue; }📝 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.for (const entry of droppableContainers.getEnabled()) { if (!entry || entry?.disabled) { continue; } const rect = droppableRects.get(entry.id); if (!rect) { continue; } const data = entry.data.current; if (data) { const { type, children } = data; if (type === "Column" && children?.length > 0) { if (active.data.current?.type !== "Column") { continue; } } } switch (event.code) { case KeyboardCode.Down: if (active.data.current?.type === "Column") { continue; } if (collisionRect.top < rect.top) { // find all droppable areas below filteredContainers.push(entry); } break; case KeyboardCode.Up: if (active.data.current?.type === "Column") { continue; } if (collisionRect.top > rect.top) { // find all droppable areas above filteredContainers.push(entry); } break; case KeyboardCode.Left: if (collisionRect.left >= rect.left + rect.width) { // find all droppable areas to left filteredContainers.push(entry); } break; case KeyboardCode.Right: // find all droppable areas to right if (collisionRect.left + collisionRect.width <= rect.left) { filteredContainers.push(entry); } break; }apps/web/store/ai-categorize-sender-queue.ts (4)
11-15: 🛠️ Refactor suggestion
Extend 'CategorizationStatus' and 'QueueItem' to handle failed tasks
To properly manage tasks that fail after all retry attempts, consider adding a
'failed'status toCategorizationStatusand an optionalerrorproperty toQueueItem.Update the type and interface:
type CategorizationStatus = "pending" | "processing" | "completed"; + | "failed"; interface QueueItem { status: CategorizationStatus; categoryId?: string; + error?: 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.type CategorizationStatus = "pending" | "processing" | "completed" | "failed"; interface QueueItem { status: CategorizationStatus; categoryId?: string; error?: string; }
81-88: 🛠️ Refactor suggestion
Remove completed and failed items from the queue to prevent memory buildup
Currently, completed and failed items remain in
aiCategorizeSenderQueueAtom, which may lead to memory issues over time. Consider removing items with terminal statuses from the queue.Adjust the code to remove items when they reach a terminal status:
jotaiStore.set(aiCategorizeSenderQueueAtom, (prev) => { const newQueue = new Map(prev); - newQueue.set(sender, { - status: "completed", - categoryId: result.categoryId || undefined, - }); + newQueue.delete(sender); return newQueue; });Alternatively, implement a cleanup mechanism that periodically clears items with statuses
'completed'or'failed'.📝 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.jotaiStore.set(aiCategorizeSenderQueueAtom, (prev) => { const newQueue = new Map(prev); newQueue.delete(sender); return newQueue; });
60-95: 🛠️ Refactor suggestion
Consider processing tasks in batches to improve performance
If the number of senders is large, creating a task for each sender individually could impact performance. Processing senders in batches may improve efficiency.
Modify the
processAiCategorizeSenderQueuefunction to handle batching:function processAiCategorizeSenderQueue({ senders }: { senders: string[] }) { - const tasks = senders.map((sender) => async () => { + const batchSize = 10; // Adjust based on optimal batch size + for (let i = 0; i < senders.length; i += batchSize) { + const batch = senders.slice(i, i + batchSize); + const tasks = batch.map((sender) => async () => { // existing task code }); + aiQueue.addAll(tasks); + } }This approach reduces the overhead of managing a large number of individual tasks.
Committable suggestion skipped: line range outside the PR's diff.
68-91:
⚠️ Potential issueHandle failures after maximum retry attempts
If
categorizeSenderActioncontinues to fail after all retries, the error is unhandled. This could lead to unnoticed failures in processing. Consider updating the item's status to'failed'and storing the error message.Modify the code to handle failures:
await pRetry( async (attemptCount) => { // existing code }, { retries: 3 }, ).catch((error) => { + jotaiStore.set(aiCategorizeSenderQueueAtom, (prev) => { + const newQueue = new Map(prev); + newQueue.set(sender, { + status: "failed", + error: error.message, + }); + return newQueue; + }); });📝 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.await pRetry( async (attemptCount) => { console.log( `Queue: aiCategorizeSender. Processing ${sender}${attemptCount > 1 ? ` (attempt ${attemptCount})` : ""}`, ); const result = await categorizeSenderAction(sender); if (isActionError(result)) { await sleep(exponentialBackoff(attemptCount, 1_000)); throw new Error(result.error); } jotaiStore.set(aiCategorizeSenderQueueAtom, (prev) => { const newQueue = new Map(prev); newQueue.set(sender, { status: "completed", categoryId: result.categoryId || undefined, }); return newQueue; }); }, { retries: 3 }, ).catch((error) => { jotaiStore.set(aiCategorizeSenderQueueAtom, (prev) => { const newQueue = new Map(prev); newQueue.set(sender, { status: "failed", error: error.message, }); return newQueue; }); });apps/web/store/archive-sender-queue.ts (3)
98-101: 🛠️ Refactor suggestion
Simplify
useArchiveSenderStatusby removing unnecessaryuseMemoThe
useMemohook may not be necessary here sincegetStatus(sender)is a simple retrieval and not computationally intensive. RemovinguseMemosimplifies the code without impacting performance.Apply this diff:
export const useArchiveSenderStatus = (sender: string) => { const getStatus = useAtomValue(archiveSenderStatusAtom); - return useMemo(() => getStatus(sender), [getStatus, sender]); + return getStatus(sender); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const useArchiveSenderStatus = (sender: string) => { const getStatus = useAtomValue(archiveSenderStatusAtom); return getStatus(sender); };
35-36:
⚠️ Potential issuePossible mismatch between expected data structure from
fetchSenderThreadsThe variable
threadsis expected to be an array, butfetchSenderThreadsmight be returning an object. Ensure thatfetchSenderThreadsreturns an array of threads. IfGetThreadsResponsecontains athreadsproperty, you should adjust the code accordingly.Apply this diff to adjust the return value of
fetchSenderThreads:async function fetchSenderThreads(sender: string) { const url = `/api/google/threads/basic?from=${encodeURIComponent(sender)}&labelId=INBOX`; const res = await fetch(url); if (!res.ok) throw new Error(`Failed to fetch threads: ${res.status} ${res.statusText}`); const data: GetThreadsResponse = await res.json(); - return data; + return data.threads; }Committable suggestion skipped: line range outside the PR's diff.
93-96: 🛠️ Refactor suggestion
Consider using
atomFamilyfor parameterized atomsInstead of returning a function from
archiveSenderStatusAtom, you could useatomFamilyfrom Jotai to create parameterized atoms. This approach can improve readability and align with best practices for state management.Here's how you could refactor the code:
-import { atom, useAtomValue } from "jotai"; +import { atom, useAtomValue, atomFamily } from "jotai"; +import { atomFamily } from "jotai/utils"; -const archiveSenderStatusAtom = atom((get) => { - const queue = get(archiveSenderQueueAtom); - return (sender: string) => queue.get(sender); -}); +const archiveSenderStatusFamily = atomFamily((sender: string) => atom((get) => { + const queue = get(archiveSenderQueueAtom); + return queue.get(sender); +})); export const useArchiveSenderStatus = (sender: string) => { - const getStatus = useAtomValue(archiveSenderStatusAtom); - return useMemo(() => getStatus(sender), [getStatus, sender]); + return useAtomValue(archiveSenderStatusFamily(sender)); };Note: Ensure you have
jotai/utilsinstalled to useatomFamily.Committable suggestion skipped: line range outside the PR's diff.
apps/web/components/kanban/BoardColumn.tsx (2)
58-60: 🛠️ Refactor suggestion
Consider making column dimensions responsive for better adaptability.
The hardcoded height of
800pxand width of350pxmay not display well on all screen sizes. Consider using responsive units or CSS techniques likemax-width,flex, or media queries to enhance responsiveness across different devices.
48-50:
⚠️ Potential issueFix incorrect accessibility attribute 'roleDescription'.
The attribute
roleDescriptionshould bearia-roledescriptionto comply with ARIA standards and ensure proper accessibility support.Apply this diff to correct the attribute:
attributes: { - roleDescription: `Column: ${column.title}`, + 'aria-roledescription': `Column: ${column.title}`, },📝 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.attributes: { 'aria-roledescription': `Column: ${column.title}`, },apps/web/app/(app)/smart-categories/page.tsx (3)
37-51:
⚠️ Potential issueAdd error handling for data fetching operations
When using
Promise.all, if any of the promises reject, it could lead to unhandled exceptions. To enhance robustness, consider wrapping the data fetching in a try-catch block or handling errors individually.Would you like assistance in implementing error handling for these asynchronous operations?
103-106:
⚠️ Potential issueHandle possible
undefinedvalues when sorting sendersWhen sorting senders by
sender.category?.name, if a sender does not have a category,sender.category?.namewill beundefined, which may affect the sorting order. To ensure consistent sorting, provide a default value.Apply this change:
sortBy( senders, - (sender) => sender.category?.name, + (sender) => sender.category?.name || '', )📝 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.emailGroups={sortBy( senders, (sender) => sender.category?.name || '', ).map((sender) => ({
35-35: 🛠️ Refactor suggestion
Refactor authentication error handling to redirect unauthenticated users
Instead of throwing an error when the user is not authenticated, consider redirecting them to the login page for a better user experience.
Apply this change:
- if (!email) throw new Error("Not authenticated"); + if (!email) redirect("/login");Committable suggestion skipped: line range outside the PR's diff.
apps/web/store/archive-queue.ts (3)
181-182:
⚠️ Potential issueReconsider removing failed threads from the queue
Currently,
removeThreadFromQueue(threadId, actionType);is called regardless of whether the action succeeded or failed. This means that threads that failed to process after all retries are removed from the queue and won't be retried later. If it's important to ensure that all threads are processed successfully, consider keeping failed threads in the queue or implementing a mechanism to retry them later.
44-49:
⚠️ Potential issueIncorrect usage of
atomWithStorage: invalid options parameterThe
atomWithStoragefunction does not accept an options parameter as a fourth argument. The correct usage isatomWithStorage(key, initialValue, storage?). Passing{ getOnInit: true }as a fourth argument may lead to unexpected behavior.Apply this diff to correct the function call:
-const queueAtom = atomWithStorage( - "gmailActionQueue", - { activeThreads: {}, totalThreads: 0 }, - createStorage(), - { getOnInit: true }, -); +const queueAtom = atomWithStorage( + "gmailActionQueue", + { activeThreads: {}, totalThreads: 0 }, + createStorage(), +);📝 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 queueAtom = atomWithStorage( "gmailActionQueue", { activeThreads: {}, totalThreads: 0 }, createStorage(), );
129-143: 🛠️ Refactor suggestion
Update
totalThreadswhen removing threads from the queueIn
removeThreadFromQueue, thetotalThreadsvalue is not updated when a thread is removed fromactiveThreads. IftotalThreadsis intended to represent the total number of threads to be processed for progress tracking, consider decrementing it when a thread is removed from the queue.Apply this diff to adjust
totalThreads:function removeThreadFromQueue(threadId: string, actionType: ActionType) { jotaiStore.set(queueAtom, (prev) => { const remainingThreads = Object.fromEntries( Object.entries(prev.activeThreads).filter( ([_key, value]) => !(value.threadId === threadId && value.actionType === actionType), ), ); return { ...prev, activeThreads: remainingThreads, + totalThreads: prev.totalThreads - 1, }; }); }📝 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 removeThreadFromQueue(threadId: string, actionType: ActionType) { jotaiStore.set(queueAtom, (prev) => { const remainingThreads = Object.fromEntries( Object.entries(prev.activeThreads).filter( ([_key, value]) => !(value.threadId === threadId && value.actionType === actionType), ), ); return { ...prev, activeThreads: remainingThreads, totalThreads: prev.totalThreads - 1, }; }); }apps/web/components/MultiSelectFilter.tsx (2)
24-34:
⚠️ Potential issueRemove unused generic type parameters
TDataandTValueThe generic type parameters
TDataandTValuein theMultiSelectFilterPropsinterface andMultiSelectFilterfunction are not utilized within the code. Removing them can simplify the code and prevent potential confusion.Apply this diff to remove the unused generic type parameters:
-interface MultiSelectFilterProps<TData, TValue> { +interface MultiSelectFilterProps { title?: string; options: { label: string; value: string; icon?: React.ComponentType<{ className?: string }>; }[]; selectedValues: Set<string>; setSelectedValues: (values: Set<string>) => void; maxDisplayedValues?: number; } -export function MultiSelectFilter<TData, TValue>({ +export function MultiSelectFilter({ title, options, selectedValues, setSelectedValues, maxDisplayedValues, }: MultiSelectFilterProps) {Also applies to: 36-42
24-34:
⚠️ Potential issueEnsure consistency in
optionstype between component and hookIn
MultiSelectFilterProps, theoptionsprop is an array of objects withlabel,value, and optionalicon. However, inuseMultiSelectFilter, theoptionsparameter is an array of strings. This inconsistency may lead to confusion or errors when integrating the hook with the component. Consider aligning theoptionstype between the component and the hook.Apply the following changes to
useMultiSelectFilterto accept the sameoptionstype and initializeselectedValuesaccordingly:-export function useMultiSelectFilter(options: string[]) { +export function useMultiSelectFilter(options: { + label: string; + value: string; + icon?: React.ComponentType<{ className?: string }>; +}[]) { const [selectedValues, setSelectedValues] = React.useState<Set<string>>( - new Set(options), + new Set(options.map((option) => option.value)), ); return { selectedValues, setSelectedValues }; }Also applies to: 156-161
apps/web/components/kanban/KanbanBoard.tsx (3)
253-268: 🛠️ Refactor suggestion
Use a safer check for window existence before using
createPortalThe current check
{"document" in window}may not reliably determine if the code is running in a browser environment. A more robust check is to verify iftypeof window !== 'undefined'. This helps prevent errors during server-side rendering.Apply this diff to update the check:
- {"document" in window && + {typeof window !== 'undefined' && createPortal( <DragOverlay> {activeColumn && ( <BoardColumn isOverlay column={activeColumn} tasks={tasks.filter( (task) => task.columnId === activeColumn.id, )} /> )} {activeTask && <TaskCard task={activeTask} isOverlay />} </DragOverlay>, document.body, )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{typeof window !== 'undefined' && createPortal( <DragOverlay> {activeColumn && ( <BoardColumn isOverlay column={activeColumn} tasks={tasks.filter( (task) => task.columnId === activeColumn.id, )} /> )} {activeTask && <TaskCard task={activeTask} isOverlay />} </DragOverlay>, document.body, )}
286-312:
⚠️ Potential issueHandle task drops in the
onDragEndfunctionThe
onDragEndfunction currently only handles dragging and dropping of columns. If a task is dragged and dropped, the function returns early and does not update thetasksstate accordingly. This means that tasks may not move correctly between columns or positions upon drop.Apply this diff to handle task drops:
function onDragEnd(event: DragEndEvent) { setActiveColumn(null); setActiveTask(null); const { active, over } = event; if (!over) return; const activeId = active.id; const overId = over.id; if (!hasDraggableData(active)) return; const activeData = active.data.current; if (activeId === overId) return; const isActiveAColumn = activeData?.type === "Column"; + const isActiveATask = activeData?.type === "Task"; - if (!isActiveAColumn) return; + if (isActiveAColumn) { setColumns((columns) => { const activeColumnIndex = columns.findIndex((col) => col.id === activeId); const overColumnIndex = columns.findIndex((col) => col.id === overId); return arrayMove(columns, activeColumnIndex, overColumnIndex); }); + } else if (isActiveATask) { + setTasks((tasks) => { + const activeIndex = tasks.findIndex((t) => t.id === activeId); + const overIndex = tasks.findIndex((t) => t.id === overId); + const activeTask = tasks[activeIndex]; + const overTask = tasks[overIndex]; + if (activeTask && overTask) { + const updatedTask = { + ...activeTask, + columnId: overTask.columnId, + }; + const updatedTasks = tasks.slice(); + updatedTasks[activeIndex] = updatedTask; + return arrayMove(updatedTasks, activeIndex, overIndex); + } + return tasks; + }); } }📝 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 onDragEnd(event: DragEndEvent) { setActiveColumn(null); setActiveTask(null); const { active, over } = event; if (!over) return; const activeId = active.id; const overId = over.id; if (!hasDraggableData(active)) return; const activeData = active.data.current; if (activeId === overId) return; const isActiveAColumn = activeData?.type === "Column"; const isActiveATask = activeData?.type === "Task"; if (isActiveAColumn) { setColumns((columns) => { const activeColumnIndex = columns.findIndex((col) => col.id === activeId); const overColumnIndex = columns.findIndex((col) => col.id === overId); return arrayMove(columns, activeColumnIndex, overColumnIndex); }); } else if (isActiveATask) { setTasks((tasks) => { const activeIndex = tasks.findIndex((t) => t.id === activeId); const overIndex = tasks.findIndex((t) => t.id === overId); const activeTask = tasks[activeIndex]; const overTask = tasks[overIndex]; if (activeTask && overTask) { const updatedTask = { ...activeTask, columnId: overTask.columnId, }; const updatedTasks = tasks.slice(); updatedTasks[activeIndex] = updatedTask; return arrayMove(updatedTasks, activeIndex, overIndex); } return tasks; }); } }
335-366:
⚠️ Potential issueAvoid mutating state directly when updating tasks in
onDragOverIn the
onDragOverfunction, you're directly mutating task objects within the state array (e.g.,activeTask.columnId = overTask.columnId;). Directly mutating state can lead to unexpected behavior in React. Instead, create new task objects when updating them to adhere to React's immutability principles.Apply these changes to prevent direct state mutation:
// When dropping a Task over another Task if (isActiveATask && isOverATask) { setTasks((tasks) => { const activeIndex = tasks.findIndex((t) => t.id === activeId); const overIndex = tasks.findIndex((t) => t.id === overId); const activeTask = tasks[activeIndex]; const overTask = tasks[overIndex]; if ( activeTask && overTask && activeTask.columnId !== overTask.columnId ) { - activeTask.columnId = overTask.columnId; + const updatedTask = { + ...activeTask, + columnId: overTask.columnId, + }; + const updatedTasks = tasks.slice(); + updatedTasks[activeIndex] = updatedTask; + return arrayMove(updatedTasks, activeIndex, overIndex - 1); } return arrayMove(tasks, activeIndex, overIndex); }); } // When dropping a Task over a Column if (isActiveATask && isOverAColumn) { setTasks((tasks) => { const activeIndex = tasks.findIndex((t) => t.id === activeId); const activeTask = tasks[activeIndex]; if (activeTask) { + const updatedTask = { + ...activeTask, + columnId: overId as string, + }; + const updatedTasks = tasks.slice(); + updatedTasks[activeIndex] = updatedTask; + return updatedTasks; - activeTask.columnId = overId as string; - return arrayMove(tasks, activeIndex, activeIndex); } return tasks; }); }📝 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.setTasks((tasks) => { const activeIndex = tasks.findIndex((t) => t.id === activeId); const overIndex = tasks.findIndex((t) => t.id === overId); const activeTask = tasks[activeIndex]; const overTask = tasks[overIndex]; if ( activeTask && overTask && activeTask.columnId !== overTask.columnId ) { const updatedTask = { ...activeTask, columnId: overTask.columnId, }; const updatedTasks = tasks.slice(); updatedTasks[activeIndex] = updatedTask; return arrayMove(updatedTasks, activeIndex, overIndex - 1); } return arrayMove(tasks, activeIndex, overIndex); }); } const isOverAColumn = overData?.type === "Column"; // Im dropping a Task over a column if (isActiveATask && isOverAColumn) { setTasks((tasks) => { const activeIndex = tasks.findIndex((t) => t.id === activeId); const activeTask = tasks[activeIndex]; if (activeTask) { const updatedTask = { ...activeTask, columnId: overId as string, }; const updatedTasks = tasks.slice(); updatedTasks[activeIndex] = updatedTask; return updatedTasks; } return tasks; }); }apps/web/app/api/google/webhook/process-history.ts (4)
339-346:
⚠️ Potential issueEnsure
message.headers.fromandmessage.headers.dateare defined before useTo prevent potential runtime errors, add checks to ensure that
message.headers.fromandmessage.headers.dateare defined before using them inhasPreviousEmailsFromSenderOrDomain.
371-371:
⚠️ Potential issueAdd null check for
message.headers.fromBefore assigning
message.headers.fromtosender, ensure it is not undefined to avoid possible null reference errors.
372-378: 🛠️ Refactor suggestion
Address potential race conditions when categorizing senders
Concurrent processing of messages from the same sender could lead to race conditions, resulting in duplicate categorization attempts. Implement a unique constraint on the
newslettertable foremail_userIdand handle duplicate key errors gracefully to prevent this issue.Consider adding a unique constraint in your Prisma schema:
model Newsletter { // existing fields email String userId String // other fields @@unique([email, userId]) }Handle the potential conflict in code:
if (!existingSender?.category) { + try { await categorizeSender(sender, user, gmail, accessToken); + } catch (error) { + if (error.code === 'P2002') { + // Unique constraint failed, another process inserted the sender + console.warn(`Sender already categorized by another process: ${sender}`); + } else { + throw error; + } + } }
376-378:
⚠️ Potential issueHandle exceptions during sender categorization
Wrap the
categorizeSendercall in a try-catch block to handle any exceptions that may occur during the categorization process, ensuring the webhook processing continues smoothly.Apply this diff to handle exceptions:
if (!existingSender?.category) { + try { await categorizeSender(sender, user, gmail, accessToken); + } catch (error) { + console.error(`Error categorizing sender: ${sender}`, error); + // Optionally, capture the exception or handle it accordingly + } }📝 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.if (!existingSender?.category) { try { await categorizeSender(sender, user, gmail, accessToken); } catch (error) { console.error(`Error categorizing sender: ${sender}`, error); // Optionally, capture the exception or handle it accordingly } }apps/web/components/GroupedTable.tsx (5)
172-176: 🛠️ Refactor suggestion
Optimize archiving senders by processing concurrently
The
onArchiveAllfunction currently archives senders sequentially, which can be slow if there are many senders to process. To improve performance, consider processing all senders concurrently usingPromise.all.Apply this diff to process senders concurrently:
const onArchiveAll = async () => { - for (const sender of senders) { - await addToArchiveSenderQueue(sender.address); - } + await Promise.all( + senders.map((sender) => addToArchiveSenderQueue(sender.address)) + ); };📝 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 onArchiveAll = async () => { await Promise.all( senders.map((sender) => addToArchiveSenderQueue(sender.address)) ); };
433-437:
⚠️ Potential issueEnsure 'categoryId' is correctly typed in 'changeSenderCategoryAction'
In the
onValueChangehandler ofSelectCategoryCell,valueis a string, but ifchangeSenderCategoryActionexpectscategoryIdas a number, you should convertvalueto an integer to prevent type mismatches.Apply this diff to fix the type:
onValueChange={async (value) => { const result = await changeSenderCategoryAction({ sender, - categoryId: value, + categoryId: parseInt(value, 10), }); if (isActionError(result)) { toastError({ description: result.error }); } else { toastSuccess({ description: "Category changed" }); } }}📝 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.onValueChange={async (value) => { const result = await changeSenderCategoryAction({ sender, categoryId: parseInt(value, 10), }); if (isActionError(result)) { toastError({ description: result.error }); } else { toastSuccess({ description: "Category changed" }); } }}
463-479:
⚠️ Potential issueAdd null checks for 'status' properties in 'ArchiveStatusCell'
In the
ArchiveStatusCellcomponent, you're usingstatus.threadsTotalandstatus.threadIds.lengthwithout checking if they are defined. To prevent potential runtime errors, add null checks before performing arithmetic operations or accessing properties.Apply this diff to add null checks:
switch (status?.status) { // ... case "processing": return ( <span className="text-blue-500"> - Archiving... {status.threadsTotal - status.threadIds.length} /{" "} - {status.threadsTotal} + Archiving... {status.threadsTotal && status.threadIds + ? status.threadsTotal - status.threadIds.length + : 0}{" "} + / {status.threadsTotal || 0} </span> ); // ... }📝 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.switch (status?.status) { case "completed": if (status.threadsTotal) { return ( <span className="text-green-500"> Archived {status.threadsTotal} emails! </span> ); } return <span className="text-gray-500">Archived</span>; case "processing": return ( <span className="text-blue-500"> Archiving... {status.threadsTotal && status.threadIds ? status.threadsTotal - status.threadIds.length : 0}{" "} / {status.threadsTotal || 0} </span> );
124-154:
⚠️ Potential issueChange accessorKey from "date" to "category" for clarity
In the
columnsdefinition, the accessorKey for the category selection column is set to"date", which is misleading since the column is actually displaying and updating the category of each email group. Changing the accessorKey to"category"improves readability and maintains consistency.Apply this diff to fix the accessorKey:
{ - accessorKey: "date", + accessorKey: "category", cell: ({ row }) => ( <Select defaultValue={row.original.category?.id.toString() || ""} onValueChange={async (value) => { const result = await changeSenderCategoryAction({ sender: row.original.address, categoryId: value, }); if (isActionError(result)) { toastError({ description: result.error }); } else { toastSuccess({ description: "Category changed" }); } }} > <SelectTrigger className="w-[180px]"> <SelectValue placeholder="Select category" /> </SelectTrigger> <SelectContent> {categories.map((category) => ( <SelectItem key={category.id} value={category.id.toString()}> {category.name} </SelectItem> ))} </SelectContent> </Select> ), },📝 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.accessorKey: "category", cell: ({ row }) => ( <Select defaultValue={row.original.category?.id.toString() || ""} onValueChange={async (value) => { const result = await changeSenderCategoryAction({ sender: row.original.address, categoryId: value, }); if (isActionError(result)) { toastError({ description: result.error }); } else { toastSuccess({ description: "Category changed" }); } }} > <SelectTrigger className="w-[180px]"> <SelectValue placeholder="Select category" /> </SelectTrigger> <SelectContent> {categories.map((category) => ( <SelectItem key={category.id} value={category.id.toString()}> {category.name} </SelectItem> ))} </SelectContent> </Select> ), }, ],
389-404:
⚠️ Potential issueAdd safety checks when accessing 'thread.messages[0]'
In the
ExpandedRowscomponent, you're accessingthread.messages[0]without verifying thatthread.messagesis not empty. Although threads typically contain messages, adding a check ensures robustness and prevents potential runtime errors ifthread.messagesis empty.Apply this diff to add safety checks:
<TableRow key={thread.id} className="bg-muted/50"> <TableCell /> <TableCell> <Link href={getGmailUrl(thread.id, sender)} target="_blank" className="hover:underline" > - {thread.messages[0].headers.subject} + {thread.messages[0]?.headers.subject || 'No Subject'} </Link> </TableCell> - <TableCell>{decodeSnippet(thread.messages[0].snippet)}</TableCell> + <TableCell>{decodeSnippet(thread.messages[0]?.snippet || '')}</TableCell> <TableCell className="text-nowrap"> - {formatShortDate(new Date(thread.messages[0].headers.date))} + {thread.messages[0]?.headers.date + ? formatShortDate(new Date(thread.messages[0].headers.date)) + : 'No Date'} </TableCell> </TableRow>📝 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.<TableRow key={thread.id} className="bg-muted/50"> <TableCell /> <TableCell> <Link href={getGmailUrl(thread.id, sender)} target="_blank" className="hover:underline" > {thread.messages[0]?.headers.subject || 'No Subject'} </Link> </TableCell> <TableCell>{decodeSnippet(thread.messages[0]?.snippet || '')}</TableCell> <TableCell className="text-nowrap"> {thread.messages[0]?.headers.date ? formatShortDate(new Date(thread.messages[0].headers.date)) : 'No Date'} </TableCell> </TableRow>apps/web/utils/actions/categorize.ts (6)
87-88: 🛠️ Refactor suggestion
Remove console.log statements to clean up production code
The
console.logstatement at line 87 is likely for debugging purposes. Consider removing it or replacing it with a proper logging mechanism for production.Apply this diff to remove the debug statement:
export const categorizeSendersAction = withActionInstrumentation( "categorizeSenders", async () => { - console.log("categorizeSendersAction");📝 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.
398-399:
⚠️ Potential issueCorrect condition in personal email domain check
The condition at line 398 is checking for a
>character, which might be a typo.Apply this diff to fix the typo:
- if (personalEmailDomains.some((domain) => sender.includes(`@${domain}>`))) + if (personalEmailDomains.some((domain) => sender.includes(`@${domain}`)))📝 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.if (personalEmailDomains.some((domain) => sender.includes(`@${domain}`))) return { sender, category: defaultCategory.UNKNOWN.name };
398-399:
⚠️ Potential issueFix condition in personal email domain check
The condition at line 398 includes an extraneous
>character, which may cause incorrect matching of email domains.Apply this diff to correct the condition:
if (personalEmailDomains.some((domain) => sender.includes(`@${domain}>`))) + if (personalEmailDomains.some((domain) => sender.includes(`@${domain}`))) return { sender, category: defaultCategory.UNKNOWN.name };Committable suggestion skipped: line range outside the PR's diff.
508-512: 🛠️ Refactor suggestion
Enhance error handling in catch block
In the catch block starting at line 508, only duplicate name errors are specifically handled. Other errors are rethrown, which may not provide sufficient feedback to the user.
Consider logging unexpected errors and returning a user-friendly message:
} catch (error) { if (isDuplicateError(error, "name")) return { error: "Category with this name already exists" }; - throw error; + console.error("Error updating category:", error); + return { error: "An unexpected error occurred while updating the category" }; }📝 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.if (isDuplicateError(error, "name")) return { error: "Category with this name already exists" }; console.error("Error updating category:", error); return { error: "An unexpected error occurred while updating the category" }; }
125-132: 🛠️ Refactor suggestion
Eliminate unnecessary console logs
Several
console.logstatements are present at lines 125, 127, and 131. These may clutter the output and are not suitable for production code.Apply this diff to remove them:
console.log("sendersResult", Array.from(sendersResult.senders.keys())); - console.log("sendersResult", Array.from(sendersResult.senders.keys())); - console.log(`Found ${sendersResult.senders.size} senders`); const senders = uniq(Array.from(sendersResult.senders.keys())); - console.log(`Found ${senders.length} unique senders`);Committable suggestion skipped: line range outside the PR's diff.
417-419:
⚠️ Potential issueEnsure correct usage of Prisma findUnique method
At line 417, the
findUniquequery uses bothidanduserIdin thewhereclause. Confirm that this matches a unique constraint or consider usingfindFirstif appropriate.You may need to adjust the query:
- where: { id: categoryId, userId: session.user.id }, + where: { id: categoryId },Alternatively, if
idis not unique globally, ensure that your schema reflects a composite unique constraint onidanduserId.Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/(app)/smart-categories/Uncategorized.tsx (2)
182-184:
⚠️ Potential issueAdd error handling for
setAutoCategorizeActionfailuresThe
onChangehandler inAutoCategorizeToggleassumes thatsetAutoCategorizeActionwill always succeed. Network issues or server errors could cause this action to fail silently, leaving the user unaware of the failure. Consider adding error handling to notify the user if the action does not complete successfully.Apply this diff to handle potential errors:
onChange={async (enabled) => { + try { await setAutoCategorizeAction(enabled); + } catch (error) { + toastError({ description: "Failed to update auto-categorize setting." }); + } }}📝 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.onChange={async (enabled) => { try { await setAutoCategorizeAction(enabled); } catch (error) { toastError({ description: "Failed to update auto-categorize setting." }); } }}
41-47:
⚠️ Potential issueAdd error handling for data fetching in
useSendersThe
useSWRInfinitehook currently doesn't handle errors when fetching data. If an API call fails, the user won't be notified, potentially leading to confusion. Implement error handling to alert the user in case of data fetching failures.Apply this diff to handle errors:
const { data, size, setSize, isLoading, error } = useSWRInfinite<UncategorizedSendersResponse>(getKey, { revalidateOnFocus: false, revalidateFirstPage: false, persistSize: true, revalidateOnMount: true, }); + useEffect(() => { + if (error) { + toastError({ description: "Failed to load senders. Please try again later." }); + } + }, [error]);Ensure you import
useEffectfrom React:import { useMemo, useCallback, useEffect } from "react";Committable suggestion skipped: line range outside the PR's diff.
1 participant
Categorize senders using AI.
Bulk archive by category and use categories as part of AI rule filters.
Summary by CodeRabbit
New Features
CategorizeWithAiButtonfor categorizing senders using AI.CreateCategoryButtonfor creating categories with a modal interface.Uncategorizedcomponent for managing uncategorized senders.KanbanBoardandTaskCardcomponents for a kanban board interface.useCategorieshook for fetching user categories.TopBarcomponent for improved layout.Bug Fixes
Improvements
Refactor
EmailListandEmailPanelcomponents for better performance and clarity.Modalcomponent structure and prop usage.Documentation