Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/web/app/(app)/clean/EmailFirehoseItem.tsx (2)
119-132: Add error recovery after toastWhen the undo action fails, you show an error toast but don't reset the
undonestate, leaving the UI stuck in the "Undoing..." state. Consider addingsetUndone(undefined)in the error case to allow the user to retry.if (isActionError(result)) { toastError({ description: result.error }); + setUndone(undefined); } else { setUndone("undone"); }
88-97: Consider adding timeout to reset "undone" stateAfter an action is successfully undone, the badge shows "Undone" indefinitely. Consider adding a timeout to reset the state after a few seconds, providing better feedback to the user that the action is complete.
const [undone, setUndone] = useState<"undoing" | "undone">(); + // Reset "undone" state after successful completion + useEffect(() => { + if (undone === "undone") { + const timeout = setTimeout(() => { + setUndone(undefined); + }, 3000); + return () => clearTimeout(timeout); + } + }, [undone]);Don't forget to import
useEffectfrom React.apps/web/app/(app)/clean/run/page.tsx (3)
8-12: Consider improving type safety and parameter validation.The component accepts
jobIdandisPreviewBatchas strings from search parameters without validation. Consider adding validation or providing defaults to handle missing or malformed inputs.export default async function CleanRunPage({ searchParams: { jobId, isPreviewBatch }, }: { - searchParams: { jobId: string; isPreviewBatch: string }; + searchParams: { jobId?: string; isPreviewBatch?: string }; }) {
36-38: Convert string parameter to boolean properly.The
isPreviewBatchparameter is a string (from URL search params) but is used directly in a boolean context. Consider explicitly converting it to a boolean.- {isPreviewBatch && ( + {isPreviewBatch === "true" && (
40-45: Consider adding a loading state for the EmailFirehose component.When threads are being processed, users would benefit from a visual indicator of progress. Consider adding a loading state especially for potentially large email lists.
<EmailFirehose threads={threads.filter((t) => t.status !== "processing")} stats={{ total, archived }} userEmail={userEmail} action={job.action} + isLoading={threads.some(t => t.status === "processing")} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(app)/clean/EmailFirehoseItem.tsx(1 hunks)apps/web/app/(app)/clean/run/page.tsx(1 hunks)
🔇 Additional comments (5)
apps/web/app/(app)/clean/EmailFirehoseItem.tsx (4)
151-162: Align getStatus function with CleanThread type definitionThe
getStatusfunction doesn't seem to handle all possible states of the CleanThread object. According to the previous review comments, theCleanThreadtype doesn't includearchiveorlabelproperties, yet these are being referenced. Additionally, the function doesn't fully handle all the possible status values ("processing","applying","completed").Please ensure that all properties being accessed exist in the CleanThread type and that all possible status values are handled appropriately.
#!/bin/bash # Check the definition of CleanThread type to verify its properties cat apps/web/utils/redis/clean.types.ts | grep -A 20 "export type CleanThread"
164-166: Verify isPending handles all status states correctlyThe
isPendingfunction checks for"processing"and"applying"status values, but doesn't address"completed". Make sure this function handles all possible states defined in the CleanThread type.
30-31: Ensure email properties are correctly typedThe code directly accesses
email.archiveandemail.label, but according to the previous review comments, these properties might not exist in the CleanThread type. Please verify these properties are properly defined in the type.
19-64: Overall component structure looks goodThe EmailItem component is well-structured with clear separation of concerns. The conditional styling and content based on email status provides good visual feedback to users.
apps/web/app/(app)/clean/run/page.tsx (1)
29-32: Good use of Promise.all for parallel database queries.The use of Promise.all to fetch counts concurrently improves performance compared to sequential queries. Well done!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/web/app/(app)/clean/CleanInstructionsStep.tsx (3)
28-28: Consider initializing instructions state from the URL.The current implementation only sets the instructions in the URL but doesn't initialize the form with existing URL values. This could lead to a disconnect between the URL state and form state when users navigate back to this page.
- const [_, setInstructions] = useQueryState("instructions", parseAsString); + const [urlInstructions, setInstructions] = useQueryState("instructions", parseAsString); + + // Initialize form with URL values + useEffect(() => { + if (urlInstructions) { + setValue("instructions", urlInstructions); + } + }, [urlInstructions, setValue]);
41-72: Consider including skip settings in the form submission.The toggle states are managed separately from the form submission. Consider including them in the form data for a more cohesive approach.
You could modify the schema and form submission to include the skip settings:
-const schema = z.object({ instructions: z.string().optional() }); +const schema = z.object({ + instructions: z.string().optional(), + skipReply: z.boolean().default(false), + skipStarred: z.boolean().default(false), + skipCalendar: z.boolean().default(false), + skipReceipt: z.boolean().default(false), + skipAttachment: z.boolean().default(false), +});Then initialize the form with the current skip settings and update the toggles to use form methods instead.
37-101: Add accessibility improvements to the component.The component could benefit from improved accessibility features, such as:
- Adding appropriate ARIA labels
- Ensuring proper heading hierarchy
- Adding descriptive text for the toggle sections
Consider adding a descriptive section heading above the toggles:
<TypographyH3>Which emails should stay in your inbox?</TypographyH3> + <p className="text-muted-foreground mt-2"> + Select categories of emails that you'd like to keep in your inbox. + </p> <div className="mt-4 grid gap-4">And ensure the form has appropriate labels:
- <form onSubmit={handleSubmit(onSubmit)} className="mt-4"> + <form onSubmit={handleSubmit(onSubmit)} className="mt-4" aria-labelledby="custom-instructions-heading"> + <h4 id="custom-instructions-heading" className="sr-only">Custom Instructions</h4>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/web/app/(app)/clean/EmailFirehose.tsx (3)
34-35: Confirm pause toggle consistency across components.Using
useEmailStream(isPaused, threads)is a solid approach. However, ensure the rest of the application (and any parent components) consistently respects the user’s pause state and thattogglePauseis invoked wherever necessary to avoid confusion if/when the feature is fully enabled.
52-56: Revisit auto-scrolling logic for large or frequently updated feeds.When new emails arrive, this effect always scrolls to the top index (0). Consider scenarios where users may want to remain at their current scroll position or see the latest entry. Possibly implement conditional auto-scroll (e.g., only if the user is near the top).
123-150: Remove or finalize commented-out pause button logic.Commented-out code can confuse maintainers and inflate technical debt if left unresolved. Consider removing the code if it is no longer needed or reintroducing it if you plan to implement the pause feature soon.
apps/web/app/(app)/clean/EmailFirehoseStats.tsx (2)
16-17: Guard against potential negative inbox counts.
const inboxCount = stats.total - stats.archived;may become negative ifstats.archivedexceedsstats.total. Consider clamping or validating these stats to ensure expected behavior.
75-95: Clamp or validate progress values to avoid graphical artifacts.The
Progresscomponent directly applies percentage values to its width. If calculations produce negative or over-100% values, it may cause unexpected rendering. Consider clamping values to[0, 100]to ensure consistent visuals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(app)/clean/EmailFirehose.tsx(1 hunks)apps/web/app/(app)/clean/EmailFirehoseStats.tsx(1 hunks)
🔇 Additional comments (3)
apps/web/app/(app)/clean/EmailFirehose.tsx (2)
17-30: Strongly typed prop definitions look good.The component's prop types (threads, stats, userEmail, action) are clearly defined, which enhances readability and reduces bugs related to improper prop usage.
70-97: Fallback UI is functional and clear.The loading indicator and messaging when
emails.length === 0provide a smooth user experience. This is a good pattern for handling empty states.apps/web/app/(app)/clean/EmailFirehoseStats.tsx (1)
27-33: Expand condition for broader action types.Currently, the label is set to either “Archived” or “Marked as read” based on
CleanAction.ARCHIVE. Future expansions may require additional conditions if newCleanActionvalues are introduced.Please verify whether your codebase has additional
CleanActionvalues that might need special handling. If so, consider adding them here or providing a default fallback label.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/web/app/(app)/clean/ConfirmationStep.tsx (1)
36-51: Consider implementing or removing the commented-out time estimation feature.The code contains a commented-out implementation of an estimated time calculation that could provide valuable feedback to users. Either:
- Implement this feature if it would improve the user experience, or
- Remove the commented code to keep the codebase clean
If you decide to implement this feature, consider refining the time estimation algorithm to be more accurate based on actual processing times from previous runs.
apps/web/app/(app)/clean/PreviewBatchCompleted.tsx (6)
28-47: Add a loading state to improve user experience.The
handleRunOnFullInboxfunction performs an asynchronous operation, but there's no visual feedback to the user while it's processing. Consider adding a loading state to the button to prevent users from clicking multiple times and to provide visual feedback during the operation.export function PreviewBatchCompleted({ total, archived, job, }: { total: number; archived: number; job: CleanupJob; }) { const [, setIsPreviewBatch] = useQueryState("isPreviewBatch", parseAsBoolean); + const [isLoading, setIsLoading] = useState(false); const handleRunOnFullInbox = async () => { + setIsLoading(true); setIsPreviewBatch(false); const result = await cleanInboxAction({ daysOld: job.daysOld, instructions: job.instructions || "", action: job.action, skips: { reply: job.skipReply, starred: job.skipStarred, calendar: job.skipCalendar, receipt: job.skipReceipt, attachment: job.skipAttachment, }, }); if (isActionError(result)) { toastError({ description: result.error }); + setIsLoading(false); return; } + setIsLoading(false); }; // ...rest of componentDon't forget to update the button:
<Button onClick={handleRunOnFullInbox} disabled={disableRunOnFullInbox || isLoading} + isLoading={isLoading} > Run on Full Inbox </Button>And add the missing import:
import { parseAsBoolean, useQueryState } from "nuqs"; + import { useState } from "react";
28-47: Consider handling successful results from cleanInboxAction.The function
cleanInboxActionis called but its successful result isn't handled or displayed to the user. This may be intentional if the action triggers a server-side process tracked elsewhere, but it's worth considering whether you should provide feedback on success.const handleRunOnFullInbox = async () => { setIsPreviewBatch(false); const result = await cleanInboxAction({ daysOld: job.daysOld, instructions: job.instructions || "", action: job.action, skips: { reply: job.skipReply, starred: job.skipStarred, calendar: job.skipCalendar, receipt: job.skipReceipt, attachment: job.skipAttachment, }, }); if (isActionError(result)) { toastError({ description: result.error }); return; } + + // Handle successful result + // For example: toast.success({ description: "Clean operation started on your full inbox" }); };
49-49: Add more explicit naming for clarity.The variable name
disableRunOnFullInboxis slightly counterintuitive since it'struewhen all emails have been processed. Consider renaming for clarity.- const disableRunOnFullInbox = total < PREVIEW_RUN_COUNT; + const allEmailsProcessed = total < PREVIEW_RUN_COUNT; + const disableRunOnFullInbox = allEmailsProcessed;Or alternatively:
- const disableRunOnFullInbox = total < PREVIEW_RUN_COUNT; + const disableRunOnFullInbox = total < PREVIEW_RUN_COUNT; // Disabled when all emails processed (fewer than preview count)
56-58: Use pluralization logic for better readability.The text "1 emails" or "1 were archived" would be grammatically incorrect. Consider adding conditional pluralization.
<CardDescription> - We processed {total} emails. {archived} were{" "} + We processed {total} {total === 1 ? "email" : "emails"}. {archived} {archived === 1 ? "was" : "were"}{" "} {job.action === CleanAction.ARCHIVE ? "archived" : "marked as read"}. </CardDescription>
66-68: Consider adding confirmation dialog before running on full inbox.Running the clean operation on a full inbox could be a significant action that affects many emails. Consider adding a confirmation dialog to prevent accidental clicks.
+ import { useState } from "react"; + import { Dialog, DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle, DialogTrigger } from "@/components/ui/dialog"; export function PreviewBatchCompleted({ /* ... existing code ... */ }) { + const [showConfirmDialog, setShowConfirmDialog] = useState(false); const handleRunOnFullInbox = async () => { + setShowConfirmDialog(false); setIsPreviewBatch(false); /* ... existing code ... */ }; /* ... existing code ... */ return ( <CardGreen className="mb-4"> {/* ... existing CardHeader ... */} <CardContent className="flex items-center gap-4"> - <Button onClick={handleRunOnFullInbox} disabled={disableRunOnFullInbox}> - Run on Full Inbox - </Button> + <Dialog open={showConfirmDialog} onOpenChange={setShowConfirmDialog}> + <DialogTrigger asChild> + <Button disabled={disableRunOnFullInbox}> + Run on Full Inbox + </Button> + </DialogTrigger> + <DialogContent> + <DialogHeader> + <DialogTitle>Confirm Full Inbox Operation</DialogTitle> + <DialogDescription> + This will {job.action === CleanAction.ARCHIVE ? "archive" : "mark as read"} emails in your entire inbox matching the criteria. Are you sure? + </DialogDescription> + </DialogHeader> + <DialogFooter> + <Button variant="outline" onClick={() => setShowConfirmDialog(false)}>Cancel</Button> + <Button onClick={handleRunOnFullInbox}>Confirm</Button> + </DialogFooter> + </DialogContent> + </Dialog> {disableRunOnFullInbox && ( /* ... existing code ... */ )} </CardContent> </CardGreen> ); }
1-77: Consider adding error boundaries or try/catch blocks for error resilience.The component currently handles server-side errors from
cleanInboxAction, but doesn't have error handling for rendering failures or other client-side errors.Consider wrapping the component with an error boundary (React's ErrorBoundary) or adding try/catch blocks to make the component more resilient against unexpected errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/app/(app)/clean/ConfirmationStep.tsx(1 hunks)apps/web/app/(app)/clean/PreviewBatchCompleted.tsx(1 hunks)apps/web/app/(app)/clean/consts.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/app/(app)/clean/consts.ts
🔇 Additional comments (4)
apps/web/app/(app)/clean/ConfirmationStep.tsx (4)
53-74: Good implementation of the cleaning process handler.The
handleStartCleaningfunction is well-structured with proper error handling and redirection. The use of thePREVIEW_RUN_COUNTconstant ensures consistency with the UI messaging.
97-101: Address the TODO comment about email processing count.There's still a TODO comment about counting only the emails being processed. This should be addressed before the feature is fully released.
Consider linking this TODO to a tracked issue in your project management system for better visibility and to ensure it gets addressed in a future iteration.
89-117: Clear and helpful user instructionsThe list of instructions provides good transparency to users about what will happen during the cleaning process. The conditional rendering based on the selected action (archive vs. mark as read) is a nice touch that improves the user experience.
1-20: Well-organized imports with appropriate dependencies.The imports are logically grouped and include all necessary dependencies for the component's functionality. The use of named imports makes the code more readable.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/web/app/(app)/clean/run/page.tsx (1)
39-39: 🛠️ Refactor suggestionEnsure threads is not undefined before filtering.
Add a null check before filtering the threads to prevent potential runtime errors.
- threads={threads.filter((t) => t.status !== "processing")} + threads={(threads || []).filter((t) => t.status !== "processing")}
🧹 Nitpick comments (1)
apps/web/app/(app)/clean/PreviewBatch.tsx (1)
44-52: Clean up commented-out code and fix description conditionals.The component contains commented-out code that should either be implemented or removed. Additionally, the description could provide better information.
- {/* <CardDescription> - We processed {total} emails. {archived} were{" "} - {job.action === CleanAction.ARCHIVE ? "archived" : "marked as read"}. - </CardDescription> */} <CardDescription> - To undo any, hover over the " - {job.action === CleanAction.ARCHIVE ? "Archive" : "Mark as read"}" - badge and click undo. + This will {job.action === CleanAction.ARCHIVE ? "archive" : "mark as read"} emails + that match your criteria. You can undo individual actions afterward. </CardDescription>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/(app)/clean/PreviewBatch.tsx(1 hunks)apps/web/app/(app)/clean/run/page.tsx(1 hunks)
🔇 Additional comments (2)
apps/web/app/(app)/clean/run/page.tsx (2)
19-19: Add error handling for thread fetching.The call to
getThreadsByJobIdlacks error handling, which could lead to unhandled exceptions if the Redis connection fails or encounters other issues.- const threads = await getThreadsByJobId(userId, jobId); + let threads = []; + try { + threads = await getThreadsByJobId(userId, jobId) || []; + } catch (error) { + console.error("Failed to fetch threads:", error); + return <CardTitle>Error fetching threads</CardTitle>; + }
23-25: Add error handling for database operations.Database operations could fail due to connection issues or other errors. Consider adding try-catch blocks to gracefully handle potential failures.
- const job = await prisma.cleanupJob.findUnique({ - where: { id: jobId, userId }, - }); + let job; + try { + job = await prisma.cleanupJob.findUnique({ + where: { id: jobId, userId }, + }); + } catch (error) { + console.error("Failed to fetch job:", error); + return <CardTitle>Error fetching job</CardTitle>; + }
1 participant
Summary by CodeRabbit
New Features
ActionSelectionStep,ConfirmationStep,CleanInstructionsStep,PreviewBatch, andEmailFirehose.Enhancements