Show bulk archive progress #237
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@elie222 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce several asynchronous functions in Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
apps/web/app/(app)/bulk-unsubscribe/ArchiveProgress.tsx (4)
8-15: LGTM: Component structure and state management are well-implemented.The component is correctly implemented as a functional component, and the use of jotai for global state management is appropriate. The calculations for progress and completion status are accurate.
Consider adding type annotations for better type safety:
export const ArchiveProgress: React.FC = () => { const { totalThreads, activeThreadIds } = useAtomValue(queueAtoms.archive); const threadsRemaining: number = Object.values(activeThreadIds).filter(Boolean).length; const totalArchived: number = totalThreads - threadsRemaining; const progress: number = (totalArchived / totalThreads) * 100; const isCompleted: boolean = progress === 100; // ... rest of the component };
17-23: LGTM: useEffect implementation is correct and user-friendly.The useEffect hook is properly used to handle the side effect of resetting total threads after completion. The 5-second delay before resetting is a good UX decision, allowing users to see the completed state.
Consider adding a cleanup function to clear the timeout if the component unmounts before the 5 seconds elapse:
useEffect(() => { if (isCompleted) { const timer = setTimeout(() => { resetTotalThreads("archive"); }, 5_000); return () => clearTimeout(timer); } }, [isCompleted]);
25-25: Consider adding a "no data" state indication.While returning null when there are no threads is a valid approach, it might be more user-friendly to display a message indicating that there are no threads to archive.
Consider replacing the early return with a conditional render:
if (!totalThreads) { return <p className="text-muted-foreground text-sm">No emails to archive.</p>; }This provides better feedback to the user about the current state.
27-57: LGTM: Render logic is well-implemented with good UX considerations.The component's render logic is well-structured, using animations to enhance the user experience. The ProgressBar is appropriately styled and updates correctly based on the archiving progress. The color change on completion provides good visual feedback, and the use of aria-live for accessibility is commendable.
Consider extracting the text content into constants or using an internationalization library for easier maintenance and potential future localization:
const ARCHIVING_IN_PROGRESS = "Archiving emails..."; const ARCHIVING_COMPLETE = "Archiving complete!"; const ARCHIVE_STATUS = "{archived} of {total} emails archived"; // Then in the JSX: <span>{isCompleted ? ARCHIVING_COMPLETE : ARCHIVING_IN_PROGRESS}</span> <span>{ARCHIVE_STATUS.replace("{archived}", totalArchived).replace("{total}", totalThreads)}</span>apps/web/app/(app)/bulk-unsubscribe/BulkUnsubscribeSection.tsx (1)
232-233: Approved: ArchiveProgress component added, but needs clarification.The
ArchiveProgresscomponent is appropriately placed within theBulkUnsubscribeSection. However, its purpose and functionality are not immediately clear from this usage.Consider adding a brief comment explaining the role of
ArchiveProgressin the bulk unsubscribe process. Additionally, if the component relies on any context or global state, it would be helpful to mention this in a comment for better maintainability.apps/web/app/(app)/bulk-unsubscribe/hooks.ts (1)
527-529: Approve filter changes and update documentation.The changes to the initial state of filters in the
useNewsletterFilterhook improve the visibility of all newsletter categories (unsubscribed, auto-archived, and approved) by default. This aligns well with the PR objective of showing bulk archive progress.Consider updating any relevant documentation or user guides to reflect this change in default filter behavior. This will ensure that users are aware of the new default view and understand how to use the filters effectively.
apps/web/utils/queue/email-actions.ts (2)
38-50: Remove UnnecessaryasyncKeywordsThe functions
markReadThreadsanddeleteEmailsare declared asasyncbut do not contain anyawaitexpressions or asynchronous operations. This might cause confusion about the function's behavior.Apply this diff to remove the unnecessary
asynckeyword:-export const markReadThreads = async ( +export const markReadThreads = ( threadIds: string[], refetch: () => void, ) => { addThreadsToQueue("markRead", threadIds, refetch); }; -export const deleteEmails = async ( +export const deleteEmails = ( threadIds: string[], refetch: () => void, ) => { addThreadsToQueue("delete", threadIds, refetch); };
76-77: Handle Empty Message Arrays SafelyThe line
const message = thread.messages?.[thread.messages.length - 1];attempts to access the last message in the thread. Ifthread.messagesis an empty array, this will result inundefined.Consider adding a check for empty message arrays:
const message = thread.messages?.[thread.messages.length - 1]; -if (!message) return; +if (!message || thread.messages.length === 0) return;Alternatively, since you're already checking if
messageis falsy, this might suffice. Ensure that this logic consistently handles all cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- apps/web/app/(app)/bulk-unsubscribe/ArchiveProgress.tsx (1 hunks)
- apps/web/app/(app)/bulk-unsubscribe/BulkUnsubscribeSection.tsx (2 hunks)
- apps/web/app/(app)/bulk-unsubscribe/hooks.ts (2 hunks)
- apps/web/providers/AppProviders.tsx (1 hunks)
- apps/web/providers/QueueProvider.tsx (0 hunks)
- apps/web/store/archive-queue.ts (1 hunks)
- apps/web/utils/queue/email-actions.ts (1 hunks)
- apps/web/utils/queue/p-queue.ts (1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/providers/QueueProvider.tsx
🧰 Additional context used
🪛 Biome
apps/web/store/archive-queue.ts
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🔇 Additional comments (9)
apps/web/utils/queue/p-queue.ts (1)
1-3: LGTM: Proper setup for client-side queue managementThe "use client" directive and the import of
PQueueare correctly implemented for client-side queue management in a Next.js application.apps/web/app/(app)/bulk-unsubscribe/ArchiveProgress.tsx (2)
1-6: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for the component's functionality, including animation, state management, and UI components. The use of custom utilities and atoms from local files promotes code reusability and maintainability.
1-58: Overall, excellent implementation with minor suggestions for improvement.The ArchiveProgress component is well-implemented, providing a clear and interactive representation of the archiving process. It effectively uses state management, animations, and accessibility features to create a good user experience. The suggestions provided in the review are minor enhancements that could further improve type safety, component lifecycle management, user feedback in edge cases, and maintainability for future development.
apps/web/app/(app)/bulk-unsubscribe/BulkUnsubscribeSection.tsx (2)
42-42: LGTM: New import for ArchiveProgress component.The import statement for the
ArchiveProgresscomponent is correctly added and follows proper conventions.
42-42: Verify integration of ArchiveProgress with bulk unsubscribe functionality.The addition of the
ArchiveProgresscomponent enhances the bulk unsubscribe feature by providing visual feedback on the archiving process. The component's integration appears clean and non-disruptive to the existing structure.To ensure full compatibility:
- Verify that
ArchiveProgresscorrectly reflects the state of bulk operations initiated in this component.- Check if any state or context providers needed by
ArchiveProgressare properly set up in parent components.- Test the component with various bulk unsubscribe scenarios to ensure it accurately represents the archiving progress.
Also applies to: 232-233
✅ Verification successful
Integration of ArchiveProgress Verified Successfully.
The
ArchiveProgresscomponent is correctly integrated into theBulkUnsubscribeSectioncomponent and functions as intended without disrupting existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of ArchiveProgress with bulk unsubscribe functionality # Test 1: Check for any shared state or context between ArchiveProgress and BulkUnsubscribeSection echo "Checking for shared state or context:" rg --type typescript "useContext|useState|useReducer" apps/web/app/\(app\)/bulk-unsubscribe/ArchiveProgress.tsx # Test 2: Look for any event handlers or props in BulkUnsubscribeSection that might update ArchiveProgress echo "Checking for relevant event handlers or props:" rg --type typescript "onArchive|updateArchiveProgress|archiveStatus" apps/web/app/\(app\)/bulk-unsubscribe/BulkUnsubscribeSection.tsx # Test 3: Check if ArchiveProgress is used consistently across the bulk unsubscribe feature echo "Checking for consistent usage of ArchiveProgress:" rg --type typescript "ArchiveProgress" apps/web/app/\(app\)/bulk-unsubscribe/Length of output: 729
Script:
#!/bin/bash # Description: Verify the integration of ArchiveProgress with bulk unsubscribe functionality # Test 1: Check for any shared state or context between ArchiveProgress and BulkUnsubscribeSection echo "Checking for shared state or context:" rg --type tsx "useContext|useState|useReducer" apps/web/app/\(app\)/bulk-unsubscribe/ArchiveProgress.tsx # Test 2: Look for any event handlers or props in BulkUnsubscribeSection that might update ArchiveProgress echo "Checking for relevant event handlers or props:" rg --type tsx "onArchive|updateArchiveProgress|archiveStatus" apps/web/app/\(app\)/bulk-unsubscribe/BulkUnsubscribeSection.tsx # Test 3: Check if ArchiveProgress is used consistently across the bulk unsubscribe feature echo "Checking for consistent usage of ArchiveProgress:" rg --type tsx "ArchiveProgress" apps/web/app/\(app\)/bulk-unsubscribe/Length of output: 687
Script:
#!/bin/bash # Description: Verify the integration of ArchiveProgress with bulk unsubscribe functionality # Test 1: Check for any shared state or context between ArchiveProgress and BulkUnsubscribeSection echo "Checking for shared state or context:" rg --glob '*.tsx' "useContext|useState|useReducer" apps/web/app/\(app\)/bulk-unsubscribe/ArchiveProgress.tsx # Test 2: Look for any event handlers or props in BulkUnsubscribeSection that might update ArchiveProgress echo "Checking for relevant event handlers or props:" rg --glob '*.tsx' "onArchive|updateArchiveProgress|archiveStatus" apps/web/app/\(app\)/bulk-unsubscribe/BulkUnsubscribeSection.tsx # Test 3: Check if ArchiveProgress is used consistently across the bulk unsubscribe feature echo "Checking for consistent usage of ArchiveProgress:" rg --glob '*.tsx' "ArchiveProgress" apps/web/app/\(app\)/bulk-unsubscribe/Length of output: 944
apps/web/app/(app)/bulk-unsubscribe/hooks.ts (2)
Line range hint
1-544: Summary of changes and their impact.The changes in this file are minimal but impactful:
- The import statement change improves code organization by moving email-related actions to a dedicated utility file.
- The modification of initial filter states in
useNewsletterFilterenhances the visibility of all newsletter categories by default, aligning with the PR objective of showing bulk archive progress.These changes contribute to better code structure and an improved user experience without introducing any apparent issues or bugs.
15-15: Approve import change and verify new location.The change in the import statement for
archiveAllSenderEmailsfrom@/providers/QueueProviderto@/utils/queue/email-actionsappears to be part of a larger refactoring effort. This change improves code organization by moving email-related actions to a dedicated utility file.To ensure the correctness of this change, please run the following verification script:
✅ Verification successful
Import change verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the new import location and its usage # Check if the new file exists if [ ! -f "apps/web/utils/queue/email-actions.ts" ]; then echo "Error: apps/web/utils/queue/email-actions.ts does not exist" exit 1 fi # Check if archiveAllSenderEmails is exported from the new file if ! grep -q "export.*archiveAllSenderEmails" "apps/web/utils/queue/email-actions.ts"; then echo "Error: archiveAllSenderEmails is not exported from apps/web/utils/queue/email-actions.ts" exit 1 fi # Check if the old import is still used anywhere in the project if grep -r "from.*@/providers/QueueProvider" apps/web; then echo "Warning: Some files still import from @/providers/QueueProvider" fi echo "Verification completed successfully"Length of output: 782
apps/web/utils/queue/email-actions.ts (2)
19-36: Ensure Consistent Invocation ofonCompleteCallbackIn the
archiveAllSenderEmailsfunction, theonCompletecallback is directly invoked when no data is found (line 32). However, when threads are found,onCompleteis passed toarchiveEmails(line 30). Confirm thatonCompleteis called appropriately after archiving is complete to maintain consistent behavior.Please verify that
archiveEmailsor the queue processing withinaddThreadsToQueuewill invokeonCompleteafter the archiving process finishes.
75-95: Verify Required Fields inthreadToRunRulesEmailIn the
threadToRunRulesEmailfunction, several email fields liketo,date,cc, and message content are commented out or omitted. IfrunRulesActiondepends on these fields, omitting them might impact its functionality.Please confirm whether all necessary email fields are included for
runRulesActionto operate correctly.
apps/web/utils/queue/p-queue.ts
Outdated
|
|
||
| import PQueue from "p-queue"; | ||
|
|
||
| export const queue = new PQueue({ concurrency: 3 }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding documentation for the concurrency setting
The queue is correctly instantiated with a concurrency of 3. However, it would be beneficial to add a comment explaining the reasoning behind this specific concurrency level.
Consider adding a comment like this:
// Set concurrency to 3 to balance between parallel processing and avoiding overwhelming the client
export const queue = new PQueue({ concurrency: 3 });Additionally, you might want to consider the following enhancements:
- Expose configuration options to allow for adjusting the concurrency level based on different environments or use cases.
- Implement basic error handling or logging for queue operations.
- Rename the file to something more descriptive of its purpose in the application, e.g.,
client-task-queue.ts.
Would you like me to provide example code for any of these suggestions?
| <QueueProvider> | ||
| <ComposeModalProvider>{props.children}</ComposeModalProvider> | ||
| </QueueProvider> | ||
| <ComposeModalProvider>{props.children}</ComposeModalProvider> |
There was a problem hiding this comment.
💡 Codebase verification
QueueProvider
There are still several files importing QueueProvider:
apps/web/components/email-list/EmailList.tsxapps/web/components/CommandK.tsxapps/web/app/(app)/automation/BulkRunRules.tsxapps/web/app/(app)/simple/SimpleList.tsx
This indicates that removing QueueProvider from AppProviders.tsx may break these components. Please ensure that QueueProvider has been appropriately replaced or removed from these files.
🔗 Analysis chain
LGTM! Verify the impact of removing QueueProvider.
The simplification of the AppProviders component by removing QueueProvider aligns with the PR objectives. This change reduces nesting and potentially improves performance.
However, please ensure:
- All functionalities previously provided by
QueueProviderare properly handled in the new state management system (as mentioned in the AI summary). - No components are relying on the context or functionalities that were provided by
QueueProvider.
To verify the removal of QueueProvider hasn't broken any dependencies, run:
Consider adding a comment in this file or updating the relevant documentation to explain why QueueProvider was removed and where its functionality has been moved. This will help future developers understand the change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to QueueProvider
rg 'QueueProvider' --type typescript --type typescriptreact
Length of output: 94
Script:
#!/bin/bash
# Search for any remaining references to QueueProvider in .ts and .tsx files
rg 'QueueProvider' --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx
Length of output: 135
Script:
#!/bin/bash
# Search for any remaining references to QueueProvider in .ts and .tsx files using correct type definitions
rg 'QueueProvider' --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -t ts -t tsx
Length of output: 451
| markRead: atomWithStorage("markReadQueue", initialQueueState), | ||
| }; | ||
|
|
||
| type ActionFunction = (threadId: string, ...args: any[]) => Promise<any>; |
There was a problem hiding this comment.
Avoid using any in ActionFunction type definition
The use of any in TypeScript is discouraged as it disables type checking and can lead to runtime errors. To enhance type safety, please specify more precise types for the function's parameters and return type.
Apply this diff to specify the argument types:
-type ActionFunction = (threadId: string, ...args: any[]) => Promise<any>;
+type ActionFunction = (threadId: string) => Promise<void>;Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Biome
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
| from: message.headers.from, | ||
| // to: message.headers.to, | ||
| // date: message.headers.date, | ||
| replyTo: message.headers["reply-to"], | ||
| // cc: message.headers.cc, | ||
| subject: message.headers.subject, | ||
| // textPlain: message.textPlain || null, | ||
| // textHtml: message.textHtml || null, | ||
| // snippet: thread.snippet, | ||
| threadId: message.threadId || "", | ||
| messageId: message.id || "", | ||
| headerMessageId: message.headers["message-id"] || "", | ||
| references: message.headers.references, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Including Additional Email Headers
The EmailForAction object omits several headers like to, date, cc, and message content. Including these fields might enhance the effectiveness of AI rule processing.
Consider uncommenting or adding the necessary fields:
const email: EmailForAction = {
from: message.headers.from,
- // to: message.headers.to,
- // date: message.headers.date,
+ to: message.headers.to,
+ date: message.headers.date,
replyTo: message.headers["reply-to"],
- // cc: message.headers.cc,
+ cc: message.headers.cc,
subject: message.headers.subject,
- // textPlain: message.textPlain || null,
- // textHtml: message.textHtml || null,
+ textPlain: message.textPlain || null,
+ textHtml: message.textHtml || null,
// snippet: thread.snippet,
threadId: message.threadId || "",
messageId: message.id || "",
headerMessageId: message.headers["message-id"] || "",
references: message.headers.references,
};Ensure that including these fields aligns with privacy policies and does not expose sensitive information.
📝 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.
| from: message.headers.from, | |
| // to: message.headers.to, | |
| // date: message.headers.date, | |
| replyTo: message.headers["reply-to"], | |
| // cc: message.headers.cc, | |
| subject: message.headers.subject, | |
| // textPlain: message.textPlain || null, | |
| // textHtml: message.textHtml || null, | |
| // snippet: thread.snippet, | |
| threadId: message.threadId || "", | |
| messageId: message.id || "", | |
| headerMessageId: message.headers["message-id"] || "", | |
| references: message.headers.references, | |
| }; | |
| from: message.headers.from, | |
| to: message.headers.to, | |
| date: message.headers.date, | |
| replyTo: message.headers["reply-to"], | |
| cc: message.headers.cc, | |
| subject: message.headers.subject, | |
| textPlain: message.textPlain || null, | |
| textHtml: message.textHtml || null, | |
| // snippet: thread.snippet, | |
| threadId: message.threadId || "", | |
| messageId: message.id || "", | |
| headerMessageId: message.headers["message-id"] || "", | |
| references: message.headers.references, | |
| }; |
| export const runAiRules = async ( | ||
| threads: Thread[], | ||
| force: boolean, | ||
| // refetch: () => void, | ||
| ) => { | ||
| // updateRunAiQueueStorage(threads, "pending"); | ||
|
|
||
| pushToAiQueueAtom(threads.map((t) => t.id)); | ||
|
|
||
| queue.addAll( | ||
| threads.map((thread) => async () => { | ||
| const message = threadToRunRulesEmail(thread); | ||
| if (!message) return; | ||
| console.log("runRulesAction", message.threadId); | ||
| const result = await runRulesAction(message, force); | ||
| console.log("result", result); | ||
| removeFromAiQueueAtom(thread.id); | ||
| // updateRunAiQueueStorage([thread], "complete"); | ||
| // refetch(); | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Add Error Handling in runAiRules Function
The runAiRules function processes threads asynchronously but lacks error handling within the queue tasks. If an error occurs during runRulesAction, it could lead to unhandled promise rejections.
Apply this diff to include error handling:
queue.addAll(
threads.map((thread) => async () => {
const message = threadToRunRulesEmail(thread);
if (!message) return;
console.log("runRulesAction", message.threadId);
+ try {
const result = await runRulesAction(message, force);
console.log("result", result);
+ } catch (error) {
+ console.error(`Error processing thread ${thread.id}:`, error);
+ // Optionally handle the error, e.g., retry logic or user notification
+ } finally {
removeFromAiQueueAtom(thread.id);
+ }
// updateRunAiQueueStorage([thread], "complete");
// refetch();
}),
);📝 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 runAiRules = async ( | |
| threads: Thread[], | |
| force: boolean, | |
| // refetch: () => void, | |
| ) => { | |
| // updateRunAiQueueStorage(threads, "pending"); | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| queue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| console.log("runRulesAction", message.threadId); | |
| const result = await runRulesAction(message, force); | |
| console.log("result", result); | |
| removeFromAiQueueAtom(thread.id); | |
| // updateRunAiQueueStorage([thread], "complete"); | |
| // refetch(); | |
| }), | |
| ); | |
| }; | |
| export const runAiRules = async ( | |
| threads: Thread[], | |
| force: boolean, | |
| // refetch: () => void, | |
| ) => { | |
| // updateRunAiQueueStorage(threads, "pending"); | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| queue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| console.log("runRulesAction", message.threadId); | |
| try { | |
| const result = await runRulesAction(message, force); | |
| console.log("result", result); | |
| } catch (error) { | |
| console.error(`Error processing thread ${thread.id}:`, error); | |
| // Optionally handle the error, e.g., retry logic or user notification | |
| } finally { | |
| removeFromAiQueueAtom(thread.id); | |
| } | |
| // updateRunAiQueueStorage([thread], "complete"); | |
| // refetch(); | |
| }), | |
| ); | |
| }; |
| export const deleteEmails = async ( | ||
| threadIds: string[], | ||
| refetch: () => void, | ||
| ) => { | ||
| addThreadsToQueue("delete", threadIds, refetch); | ||
| }; |
There was a problem hiding this comment.
Implement error handling in deleteEmails function
To maintain consistency with other functions and improve robustness, the deleteEmails function should include error handling.
Apply this diff to add error handling:
export const deleteEmails = async (
threadIds: string[],
refetch: () => void,
) => {
+ try {
addThreadsToQueue("delete", threadIds, refetch);
+ } catch (error) {
+ console.error("Error deleting emails:", error);
+ // Optionally: Implement user notification or error recovery logic
+ }
};This change ensures consistent error handling across similar functions and prevents unhandled promise rejections.
📝 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 deleteEmails = async ( | |
| threadIds: string[], | |
| refetch: () => void, | |
| ) => { | |
| addThreadsToQueue("delete", threadIds, refetch); | |
| }; | |
| export const deleteEmails = async ( | |
| threadIds: string[], | |
| refetch: () => void, | |
| ) => { | |
| try { | |
| addThreadsToQueue("delete", threadIds, refetch); | |
| } catch (error) { | |
| console.error("Error deleting emails:", error); | |
| // Optionally: Implement user notification or error recovery logic | |
| } | |
| }; |
| export const markReadThreads = async ( | ||
| threadIds: string[], | ||
| refetch: () => void, | ||
| ) => { | ||
| addThreadsToQueue("markRead", threadIds, refetch); | ||
| }; |
There was a problem hiding this comment.
Add error handling to markReadThreads function
Similar to the archiveEmails function, markReadThreads should include error handling to maintain consistency and improve robustness.
Apply this diff to add error handling:
export const markReadThreads = async (
threadIds: string[],
refetch: () => void,
) => {
+ try {
addThreadsToQueue("markRead", threadIds, refetch);
+ } catch (error) {
+ console.error("Error marking threads as read:", error);
+ // Optionally: Implement user notification or error recovery logic
+ }
};This change will ensure consistent error handling across similar functions and prevent unhandled promise rejections.
📝 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 markReadThreads = async ( | |
| threadIds: string[], | |
| refetch: () => void, | |
| ) => { | |
| addThreadsToQueue("markRead", threadIds, refetch); | |
| }; | |
| export const markReadThreads = async ( | |
| threadIds: string[], | |
| refetch: () => void, | |
| ) => { | |
| try { | |
| addThreadsToQueue("markRead", threadIds, refetch); | |
| } catch (error) { | |
| console.error("Error marking threads as read:", error); | |
| // Optionally: Implement user notification or error recovery logic | |
| } | |
| }; |
| function threadToRunRulesEmail(thread: Thread): EmailForAction | undefined { | ||
| const message = thread.messages?.[thread.messages.length - 1]; | ||
| if (!message) return; | ||
| const email: EmailForAction = { | ||
| from: message.headers.from, | ||
| replyTo: message.headers["reply-to"], | ||
| subject: message.headers.subject, | ||
| threadId: message.threadId || "", | ||
| messageId: message.id || "", | ||
| headerMessageId: message.headers["message-id"] || "", | ||
| references: message.headers.references, | ||
| }; | ||
|
|
||
| return email; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider including additional email headers in threadToRunRulesEmail function
The threadToRunRulesEmail function currently omits several potentially useful email headers. Including additional headers might enhance the effectiveness of AI rule processing.
Consider adding more email headers to the EmailForAction object:
function threadToRunRulesEmail(thread: Thread): EmailForAction | undefined {
const message = thread.messages?.[thread.messages.length - 1];
if (!message) return;
const email: EmailForAction = {
from: message.headers.from,
+ to: message.headers.to,
+ cc: message.headers.cc,
+ date: message.headers.date,
replyTo: message.headers["reply-to"],
subject: message.headers.subject,
+ textPlain: message.textPlain || null,
+ textHtml: message.textHtml || null,
threadId: message.threadId || "",
messageId: message.id || "",
headerMessageId: message.headers["message-id"] || "",
references: message.headers.references,
};
return email;
}However, before implementing this change, please consider the following:
- Ensure that including these additional fields aligns with your privacy policy and does not expose sensitive information.
- Verify that the
EmailForActiontype supports these additional fields. - Assess the impact on performance and data usage, especially for clients with limited resources.
If you decide to include these fields, make sure to update any relevant documentation and ensure that the AI rules system can effectively utilize this additional information.
Committable suggestion was skipped due to low confidence.
| export const runAiRules = async (threads: Thread[], force: boolean) => { | ||
| pushToAiQueueAtom(threads.map((t) => t.id)); | ||
|
|
||
| queue.addAll( | ||
| threads.map((thread) => async () => { | ||
| const message = threadToRunRulesEmail(thread); | ||
| if (!message) return; | ||
| await runRulesAction(message, force); | ||
| removeFromAiQueueAtom(thread.id); | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Add error handling to queue tasks in runAiRules function
The runAiRules function processes threads asynchronously but lacks error handling within the queue tasks. This could lead to unhandled promise rejections if an error occurs during runRulesAction.
Apply this diff to add error handling to the queue tasks:
export const runAiRules = async (threads: Thread[], force: boolean) => {
pushToAiQueueAtom(threads.map((t) => t.id));
queue.addAll(
threads.map((thread) => async () => {
const message = threadToRunRulesEmail(thread);
if (!message) return;
+ try {
await runRulesAction(message, force);
+ } catch (error) {
+ console.error(`Error processing thread ${thread.id}:`, error);
+ // Optionally: Implement user notification or error recovery logic
+ } finally {
removeFromAiQueueAtom(thread.id);
+ }
}),
);
};This change ensures that errors during thread processing are caught and logged, and that removeFromAiQueueAtom is always called, even if an error occurs.
📝 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 runAiRules = async (threads: Thread[], force: boolean) => { | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| queue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| await runRulesAction(message, force); | |
| removeFromAiQueueAtom(thread.id); | |
| }), | |
| ); | |
| }; | |
| export const runAiRules = async (threads: Thread[], force: boolean) => { | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| queue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| try { | |
| await runRulesAction(message, force); | |
| } catch (error) { | |
| console.error(`Error processing thread ${thread.id}:`, error); | |
| // Optionally: Implement user notification or error recovery logic | |
| } finally { | |
| removeFromAiQueueAtom(thread.id); | |
| } | |
| }), | |
| ); | |
| }; |
| export const archiveAllSenderEmails = async ( | ||
| from: string, | ||
| onComplete: () => void, | ||
| ) => { | ||
| // 1. search gmail for messages from sender | ||
| const url = `/api/google/threads/basic?from=${from}&labelId=INBOX`; | ||
| const res = await fetch(url); | ||
| const data: GetThreadsResponse = await res.json(); | ||
|
|
||
| // 2. archive messages | ||
| if (data?.length) { | ||
| archiveEmails(data.map((t) => t.id).filter(isDefined), onComplete); | ||
| } else { | ||
| onComplete(); | ||
| } | ||
|
|
||
| return data; | ||
| }; |
There was a problem hiding this comment.
Implement comprehensive error handling in archiveAllSenderEmails function
The archiveAllSenderEmails function performs a network request without proper error handling. This could lead to unhandled promise rejections or unexpected behavior.
Apply this diff to add comprehensive error handling:
export const archiveAllSenderEmails = async (
from: string,
onComplete: () => void,
) => {
+ try {
// 1. search gmail for messages from sender
const url = `/api/google/threads/basic?from=${from}&labelId=INBOX`;
const res = await fetch(url);
+ if (!res.ok) {
+ throw new Error(`HTTP error! status: ${res.status}`);
+ }
const data: GetThreadsResponse = await res.json();
// 2. archive messages
if (data?.length) {
archiveEmails(data.map((t) => t.id).filter(isDefined), onComplete);
} else {
onComplete();
}
return data;
+ } catch (error) {
+ console.error("Error in archiveAllSenderEmails:", error);
+ onComplete(); // Ensure onComplete is called even if an error occurs
+ return []; // Return an empty array to maintain the expected return type
+ }
};This change adds error handling for the network request and overall function execution, ensuring that onComplete is always called and maintaining the expected return type.
📝 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 archiveAllSenderEmails = async ( | |
| from: string, | |
| onComplete: () => void, | |
| ) => { | |
| // 1. search gmail for messages from sender | |
| const url = `/api/google/threads/basic?from=${from}&labelId=INBOX`; | |
| const res = await fetch(url); | |
| const data: GetThreadsResponse = await res.json(); | |
| // 2. archive messages | |
| if (data?.length) { | |
| archiveEmails(data.map((t) => t.id).filter(isDefined), onComplete); | |
| } else { | |
| onComplete(); | |
| } | |
| return data; | |
| }; | |
| export const archiveAllSenderEmails = async ( | |
| from: string, | |
| onComplete: () => void, | |
| ) => { | |
| try { | |
| // 1. search gmail for messages from sender | |
| const url = `/api/google/threads/basic?from=${from}&labelId=INBOX`; | |
| const res = await fetch(url); | |
| if (!res.ok) { | |
| throw new Error(`HTTP error! status: ${res.status}`); | |
| } | |
| const data: GetThreadsResponse = await res.json(); | |
| // 2. archive messages | |
| if (data?.length) { | |
| archiveEmails(data.map((t) => t.id).filter(isDefined), onComplete); | |
| } else { | |
| onComplete(); | |
| } | |
| return data; | |
| } catch (error) { | |
| console.error("Error in archiveAllSenderEmails:", error); | |
| onComplete(); // Ensure onComplete is called even if an error occurs | |
| return []; // Return an empty array to maintain the expected return type | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
apps/web/utils/queue/email-action-queue.ts (1)
5-6: LGTM: Well-configured email action queueThe creation and export of the
emailActionQueuewith a concurrency of 3 is well-implemented. This setup will help prevent overwhelming the Gmail API while allowing for efficient processing of email actions.Consider expanding the comment slightly to provide more context:
-// Avoid overwhelming Gmail API +// Limit concurrency to 3 to avoid overwhelming the Gmail API and potential rate limitingapps/web/app/(app)/simple/SimpleList.tsx (2)
Line range hint
66-79: LGTM. Consider memoizing filtered arrays for performance.The changes introduce well-structured logic for managing message states:
toArchivearray correctly excludes messages marked to be handled later.handleUnsubscribefunction properly updates the component's state.filteredMessagesarray further refines the list by excluding unsubscribed messages.These changes improve the functionality and user experience of the component.
Consider memoizing
toArchiveandfilteredMessagesarrays usinguseMemoto optimize performance, especially ifprops.messagesis large:const toArchive = useMemo(() => props.messages .filter((m) => !toHandleLater[m.id]) .map((m) => m.threadId), [props.messages, toHandleLater] ); const filteredMessages = useMemo(() => props.messages.filter( (m) => !toHandleLater[m.id] && !unsubscribed.has(m.id) ), [props.messages, toHandleLater, unsubscribed] );
Line range hint
103-127: LGTM. Consider adding error handling for archiveEmails.The updated onClick handler for the HoverButton improves the application flow:
- It now calls
archiveEmailswith thetoArchivearray.- The navigation logic has been refined to handle different scenarios based on
nextPageTokenand the current category.These changes align with the AI-generated summary and enhance the user experience.
Consider adding error handling for the
archiveEmailsfunction call:archiveEmails(toArchive, () => {}) .catch((error) => { console.error('Failed to archive emails:', error); // Optionally, show an error message to the user });This will ensure that any failures in the archiving process are logged and can be addressed, improving the robustness of the application.
apps/web/store/archive-queue.ts (1)
36-36: Ensure consistent action function references inactionMapIn the
actionMap, thearchiveanddeleteactions reference their respective action functions directly, while themarkReadaction uses an inline arrow function to callmarkReadThreadAction(threadId, true). For consistency and clarity, consider defining all actions in the same manner.You could modify
markReadThreadActionto have a default parameter value, allowing you to reference it directly:- export function markReadThreadAction(threadId: string, isRead: boolean) { /* ... */ } + export function markReadThreadAction(threadId: string, isRead: boolean = true) { /* ... */ }Then update the
actionMap:- markRead: (threadId: string) => markReadThreadAction(threadId, true), + markRead: markReadThreadAction,Alternatively, if changing the default parameter isn't feasible, you might wrap the other actions similarly for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- apps/web/app/(app)/automation/BulkRunRules.tsx (1 hunks)
- apps/web/app/(app)/simple/SimpleList.tsx (1 hunks)
- apps/web/components/CommandK.tsx (1 hunks)
- apps/web/components/email-list/EmailList.tsx (1 hunks)
- apps/web/store/archive-queue.ts (1 hunks)
- apps/web/utils/queue/email-action-queue.ts (1 hunks)
- apps/web/utils/queue/email-actions.ts (1 hunks)
🧰 Additional context used
🪛 Biome
apps/web/store/archive-queue.ts
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
[error] 31-31: Unexpected any. Specify a different type.
any disables many type checking rules. Its use should be avoided.
(lint/suspicious/noExplicitAny)
🔇 Additional comments (11)
apps/web/utils/queue/email-action-queue.ts (2)
1-1: LGTM: Appropriate use of "use client" directiveThe "use client" directive is correctly placed at the top of the file. This ensures that the email action queue will be rendered on the client-side, which is suitable for handling user-initiated email actions responsively.
1-6: Great implementation of email action queueThis new file successfully implements a client-side email action queue using PQueue, which directly supports the PR objective of "Show bulk archive progress". The queue with a concurrency limit of 3 will allow for controlled processing of bulk email actions, including archiving, while preventing API overload.
Some key points:
- The "use client" directive ensures client-side rendering.
- PQueue is appropriately used for managing asynchronous actions.
- The concurrency limit is well-chosen and explained.
This implementation lays a solid foundation for managing bulk email actions efficiently and responsively.
apps/web/utils/queue/email-actions.ts (4)
26-31:⚠️ Potential issueImplement error handling in
deleteEmailsfunctionTo maintain consistency with other functions and improve robustness, the
deleteEmailsfunction should include error handling.Apply this diff to add error handling:
export const deleteEmails = async ( threadIds: string[], refetch: () => void, ) => { + try { addThreadsToQueue("delete", threadIds, refetch); + } catch (error) { + console.error("Error deleting emails:", error); + // Optionally: Implement user notification or error recovery logic + } };This change ensures consistent error handling across similar functions and prevents unhandled promise rejections.
Likely invalid or redundant comment.
19-24:⚠️ Potential issueAdd error handling to
markReadThreadsfunctionSimilar to the
archiveEmailsfunction,markReadThreadsshould include error handling to maintain consistency and improve robustness.Apply this diff to add error handling:
export const markReadThreads = async ( threadIds: string[], refetch: () => void, ) => { + try { addThreadsToQueue("markRead", threadIds, refetch); + } catch (error) { + console.error("Error marking threads as read:", error); + // Optionally: Implement user notification or error recovery logic + } };This change will ensure consistent error handling across similar functions and prevent unhandled promise rejections.
Likely invalid or redundant comment.
33-50:⚠️ Potential issueImplement comprehensive error handling in
archiveAllSenderEmailsfunctionThe
archiveAllSenderEmailsfunction performs network requests without proper error handling. This could lead to unhandled promise rejections or unexpected behavior.Apply this diff to add comprehensive error handling:
export const archiveAllSenderEmails = async ( from: string, onComplete: () => void, ) => { + try { // 1. search gmail for messages from sender const url = `/api/google/threads/basic?from=${from}&labelId=INBOX`; const res = await fetch(url); + if (!res.ok) { + throw new Error(`HTTP error! status: ${res.status}`); + } const data: GetThreadsResponse = await res.json(); // 2. archive messages if (data?.length) { archiveEmails(data.map((t) => t.id).filter(isDefined), onComplete); } else { onComplete(); } return data; + } catch (error) { + console.error("Error in archiveAllSenderEmails:", error); + onComplete(); // Ensure onComplete is called even if an error occurs + return []; // Return an empty array to maintain the expected return type + } };This change adds error handling for the network request and overall function execution, ensuring that
onCompleteis always called and maintaining the expected return type.Likely invalid or redundant comment.
65-79: 🛠️ Refactor suggestionConsider including additional email headers in
threadToRunRulesEmailfunctionThe
threadToRunRulesEmailfunction currently omits several potentially useful email headers. Including additional headers might enhance the effectiveness of AI rule processing.Consider adding more email headers to the
EmailForActionobject:function threadToRunRulesEmail(thread: Thread): EmailForAction | undefined { const message = thread.messages?.[thread.messages.length - 1]; if (!message) return; const email: EmailForAction = { from: message.headers.from, + to: message.headers.to, + cc: message.headers.cc, + date: message.headers.date, replyTo: message.headers["reply-to"], subject: message.headers.subject, + textPlain: message.textPlain || null, + textHtml: message.textHtml || null, threadId: message.threadId || "", messageId: message.id || "", headerMessageId: message.headers["message-id"] || "", references: message.headers.references, }; return email; }However, before implementing this change, please consider the following:
- Ensure that including these additional fields aligns with your privacy policy and does not expose sensitive information.
- Verify that the
EmailForActiontype supports these additional fields.- Assess the impact on performance and data usage, especially for clients with limited resources.
If you decide to include these fields, make sure to update any relevant documentation and ensure that the AI rules system can effectively utilize this additional information.
Likely invalid or redundant comment.
apps/web/components/CommandK.tsx (1)
19-19: Approve import change and verify consistency across the project.The change in the import statement for
archiveEmailsfrom "@/providers/QueueProvider" to "@/utils/queue/email-actions" is a good refactoring decision. It likely improves code organization by moving email-related actions to a dedicated utility file.To ensure consistency across the project, please run the following script to check for any remaining imports from the old location:
✅ Verification successful
Import change verified and approved.
The update to the import statement for
archiveEmailssuccessfully removes any remaining imports from@/providers/QueueProvider, ensuring consistency across the project.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports from the old QueueProvider location # Test: Search for imports from the old location rg --type typescript --type tsx 'from\s+[\'"]@/providers/QueueProvider[\'"]' # Note: If this returns any results, those files may need to be updated as well.Length of output: 93
Script:
#!/bin/bash # Description: Check for any remaining imports from the old QueueProvider location # Test: Search for imports from the old location rg --type typescript --type tsx "from\s+['\"]@/providers/QueueProvider['\"]" # Note: If this returns any results, those files may need to be updated as well.Length of output: 117
Script:
#!/bin/bash # Description: Check for any remaining imports from the old QueueProvider location # Test: Search for imports from the old location rg --type ts --type tsx "from\s+['\"]@/providers/QueueProvider['\"]" # Note: If this returns any results, those files may need to be updated as well.Length of output: 102
Script:
#!/bin/bash # Description: Check for any remaining imports from the old QueueProvider location # Test: Search for imports from the old location within TypeScript and TSX files rg -g "*.{ts,tsx}" "from\s+['\"]@/providers/QueueProvider['\"]" # Note: If this returns any results, those files may need to be updated as well.Length of output: 69
apps/web/app/(app)/automation/BulkRunRules.tsx (1)
13-13: LGTM. Verify import path and check for consistency across the project.The change in the import statement for
runAiRuleslooks good. This refactoring likely improves code organization.To ensure the change is correct and consistent:
- Verify that the new import path is correct and the
runAiRulesfunction exists in the new location.- Check if similar changes are needed in other files that might be importing
runAiRulesfrom the old location.Run the following script to verify the import and check for potential inconsistencies:
If Test 1 returns results, it confirms the new import path is being used. If Test 2 returns results, it indicates that there might be other files still using the old import path, which should be updated for consistency.
apps/web/app/(app)/simple/SimpleList.tsx (1)
20-20: LGTM. Verify the new import path.The import statement for
archiveEmailshas been updated to a new path. This change is consistent with the refactoring mentioned in the AI-generated summary.To ensure the new import path is correct and consistent across the project, run the following command:
apps/web/components/email-list/EmailList.tsx (2)
33-33: LGTM: Improved import organization for email actionsThe change in import statement for email action functions from '@/providers/QueueProvider' to '@/utils/queue/email-actions' is a good refactoring decision. It improves code organization by moving these utility functions to a more appropriate and specific module.
Line range hint
1-594: Overall assessment: Successful refactoring with minimal impactThe primary change in this file is the update to the import statement for email action functions. This refactoring improves code organization without affecting the functionality of the
EmailListcomponent. The rest of the file remains unchanged, indicating that the existing usage of these functions is compatible with the new import location.A few observations and suggestions:
- The component logic remains intact, which is good for maintaining consistency.
- Consider adding unit tests (if not already present) to ensure the refactored functions work as expected within this component.
- It might be beneficial to review other components that may be using these email action functions to ensure consistent updates across the codebase.
To ensure consistency across the codebase, let's check for any remaining imports from the old location:
| @@ -0,0 +1,6 @@ | |||
| "use client"; | |||
|
|
|||
| import PQueue from "p-queue"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: Missing "p-queue" Dependency
The p-queue library is imported in apps/web/utils/queue/email-action-queue.ts but is not listed in package.json dependencies. Please add "p-queue" to the project's dependencies to ensure proper functionality.
🔗 Analysis chain
LGTM: Correct import of PQueue
The import statement for PQueue from "p-queue" is correct. This library is well-suited for managing asynchronous email actions.
Please ensure that "p-queue" is listed in the project's dependencies. You can verify this by running the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if p-queue is listed in package.json
if grep -q '"p-queue":' package.json; then
echo "p-queue is listed in package.json"
else
echo "p-queue is not listed in package.json"
fi
Length of output: 120
| export const archiveEmails = async ( | ||
| threadIds: string[], | ||
| refetch?: () => void, | ||
| ) => { | ||
| addThreadsToQueue("archive", threadIds, refetch); | ||
| }; |
There was a problem hiding this comment.
Implement error handling in archiveEmails function
To improve robustness and consistency with best practices, consider adding error handling to the archiveEmails function.
Apply this diff to add error handling:
export const archiveEmails = async (
threadIds: string[],
refetch?: () => void,
) => {
+ try {
addThreadsToQueue("archive", threadIds, refetch);
+ } catch (error) {
+ console.error("Error archiving emails:", error);
+ // Optionally: Implement user notification or error recovery logic
+ }
};This change will prevent unhandled promise rejections and provide better visibility into potential issues.
📝 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 archiveEmails = async ( | |
| threadIds: string[], | |
| refetch?: () => void, | |
| ) => { | |
| addThreadsToQueue("archive", threadIds, refetch); | |
| }; | |
| export const archiveEmails = async ( | |
| threadIds: string[], | |
| refetch?: () => void, | |
| ) => { | |
| try { | |
| addThreadsToQueue("archive", threadIds, refetch); | |
| } catch (error) { | |
| console.error("Error archiving emails:", error); | |
| // Optionally: Implement user notification or error recovery logic | |
| } | |
| }; |
| export const runAiRules = async (threads: Thread[], force: boolean) => { | ||
| pushToAiQueueAtom(threads.map((t) => t.id)); | ||
|
|
||
| emailActionQueue.addAll( | ||
| threads.map((thread) => async () => { | ||
| const message = threadToRunRulesEmail(thread); | ||
| if (!message) return; | ||
| await runRulesAction(message, force); | ||
| removeFromAiQueueAtom(thread.id); | ||
| }), | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Add error handling to queue tasks in runAiRules function
The runAiRules function processes threads asynchronously but lacks error handling within the queue tasks. This could lead to unhandled promise rejections if an error occurs during runRulesAction.
Apply this diff to add error handling to the queue tasks:
export const runAiRules = async (threads: Thread[], force: boolean) => {
pushToAiQueueAtom(threads.map((t) => t.id));
emailActionQueue.addAll(
threads.map((thread) => async () => {
const message = threadToRunRulesEmail(thread);
if (!message) return;
+ try {
await runRulesAction(message, force);
+ } catch (error) {
+ console.error(`Error processing thread ${thread.id}:`, error);
+ // Optionally: Implement user notification or error recovery logic
+ } finally {
removeFromAiQueueAtom(thread.id);
+ }
}),
);
};This change ensures that errors during thread processing are caught and logged, and that removeFromAiQueueAtom is always called, even if an error occurs.
📝 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 runAiRules = async (threads: Thread[], force: boolean) => { | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| emailActionQueue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| await runRulesAction(message, force); | |
| removeFromAiQueueAtom(thread.id); | |
| }), | |
| ); | |
| }; | |
| export const runAiRules = async (threads: Thread[], force: boolean) => { | |
| pushToAiQueueAtom(threads.map((t) => t.id)); | |
| emailActionQueue.addAll( | |
| threads.map((thread) => async () => { | |
| const message = threadToRunRulesEmail(thread); | |
| if (!message) return; | |
| try { | |
| await runRulesAction(message, force); | |
| } catch (error) { | |
| console.error(`Error processing thread ${thread.id}:`, error); | |
| // Optionally: Implement user notification or error recovery logic | |
| } finally { | |
| removeFromAiQueueAtom(thread.id); | |
| } | |
| }), | |
| ); | |
| }; |
| threadIds.map((threadId) => async () => { | ||
| await action(threadId); | ||
|
|
||
| // remove completed thread from activeThreadIds | ||
| jotaiStore.set(queueAtom, (prev) => { | ||
| const { [threadId]: _, ...remainingThreads } = prev.activeThreadIds; | ||
| return { | ||
| ...prev, | ||
| activeThreadIds: remainingThreads, | ||
| }; | ||
| }); | ||
|
|
||
| refetch?.(); | ||
| }), |
There was a problem hiding this comment.
Add error handling for asynchronous actions
Currently, if the action(threadId) call fails due to errors like network issues or server failures, the error is unhandled. This could lead to inconsistent application state, such as threads being removed from activeThreadIds despite the action not completing successfully, and users not being notified of the failure.
Consider wrapping the action call in a try-catch block to handle potential errors appropriately.
Apply this diff to add error handling:
emailActionQueue.addAll(
threadIds.map((threadId) => async () => {
- await action(threadId);
+ try {
+ await action(threadId);
+ } catch (error) {
+ // Handle the error, e.g., log it or update the UI
+ console.error(`Failed to process ${queueType} action for thread ${threadId}:`, error);
+ // Optionally, decide whether to keep the threadId in activeThreadIds or implement retry logic
+ return;
+ }
// remove completed thread from activeThreadIds
jotaiStore.set(queueAtom, (prev) => {
const { [threadId]: _, ...remainingThreads } = prev.activeThreadIds;
return {
...prev,
activeThreadIds: remainingThreads,
};
});
refetch?.();
}),
);📝 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.
| threadIds.map((threadId) => async () => { | |
| await action(threadId); | |
| // remove completed thread from activeThreadIds | |
| jotaiStore.set(queueAtom, (prev) => { | |
| const { [threadId]: _, ...remainingThreads } = prev.activeThreadIds; | |
| return { | |
| ...prev, | |
| activeThreadIds: remainingThreads, | |
| }; | |
| }); | |
| refetch?.(); | |
| }), | |
| threadIds.map((threadId) => async () => { | |
| try { | |
| await action(threadId); | |
| } catch (error) { | |
| // Handle the error, e.g., log it or update the UI | |
| console.error(`Failed to process ${queueType} action for thread ${threadId}:`, error); | |
| // Optionally, decide whether to keep the threadId in activeThreadIds or implement retry logic | |
| return; | |
| } | |
| // remove completed thread from activeThreadIds | |
| jotaiStore.set(queueAtom, (prev) => { | |
| const { [threadId]: _, ...remainingThreads } = prev.activeThreadIds; | |
| return { | |
| ...prev, | |
| activeThreadIds: remainingThreads, | |
| }; | |
| }); | |
| refetch?.(); | |
| }), |
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor
Bug Fixes