Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces new functionality to manage email thread states. An asynchronous function is added to toggle the star status of threads. Additionally, several UI components are implemented to provide context menus and wrappers for thread display, along with enhancements to folder-based logic. Localization strings have been extended to cover new actions, and identifiers related to favorites have been updated. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TS as toggleStar Function
participant AD as Active Driver
participant DB as Thread Database
U->>TS: Call toggleStar({ids})
TS->>AD: Get active driver
TS->>TS: Normalize IDs and validate input
TS->>DB: Fetch thread(s) by IDs
alt Thread found
TS->>TS: Check if thread is already starred
alt Already starred
TS->>DB: Remove 'STARRED' label
else
TS->>DB: Add 'STARRED' label
end
TS->>U: Return success response
else Thread not found
TS->>U: Return error "Thread not found"
end
sequenceDiagram
participant U as User
participant TCM as ThreadContextMenu
participant TW as ThreadWrapper
U->>TCM: Right-click to open context menu
TCM->>TCM: Render dynamic actions based on thread state and folder type
U->>TCM: Select "Toggle Favorite" action
TCM->>TW: Trigger refresh callback/update state (invoking toggle behavior)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 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: 2
🧹 Nitpick comments (1)
apps/mail/components/context/thread-context.tsx (1)
271-272: Implement trash-moving functionality or remove the placeholder.Line 272 includes a TODO for moving items to trash. This is currently disabled. If you plan to support it, add an implementation that calls a backend action (similar to
moveThreadsTo) or remove the placeholder action if not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/mail/actions/mail.ts(1 hunks)apps/mail/components/context/thread-context.tsx(1 hunks)apps/mail/components/mail/mail-list.tsx(7 hunks)apps/mail/components/ui/context-menu.tsx(1 hunks)apps/mail/locales/en.json(2 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/mail/actions/mail.ts (1)
apps/mail/app/api/driver/google.ts (1)
driver(81-567)
apps/mail/components/context/thread-context.tsx (4)
apps/mail/components/mail/use-mail.ts (1)
useMail(15-17)apps/mail/hooks/use-threads.ts (1)
useThreads(77-129)apps/mail/lib/thread-actions.ts (2)
ThreadDestination(4-4)moveThreadsTo(43-81)apps/mail/actions/mail.ts (3)
toggleStar(115-142)markAsRead(45-55)markAsUnread(57-67)
🔇 Additional comments (23)
apps/mail/components/context/thread-context.tsx (1)
92-97: Confirm bulk star toggle logic.When multiple threads are selected,
isStarredis set totrueonly if every selected thread is starred (line 94). Verify that this behavior is desired. If you want partial star toggling (e.g., some are starred, some are not), you may need more nuanced handling instead of aggregating all threads under a single boolean.apps/mail/locales/en.json (2)
15-31: New action messages look consistent.These new strings provide clear user feedback for email movement, favorite toggles, and read/unread status. Good job integrating them into the action workflow. No issues spotted.
239-240: Favoriting strings align well with “STARRED” label usage.“addFavorite” and “removeFavorite” match the rest of the star/favorite feature, ensuring consistent terminology in the UI. No issues found.
apps/mail/components/mail/mail-list.tsx (8)
13-13: Added StarOff icon import.The import for
StarOfficon from lucide-react is added, which is likely used in the ThreadContextMenu component for toggling star status of email threads.
24-24: Good use of constants for folder identification.Using the FOLDERS constant from utils rather than hardcoded strings is a good practice for consistency and maintainability.
38-38: Imported ThreadContextMenu for right-click functionality.This import brings in the context menu component specifically designed for thread actions, supporting the new functionality described in the PR.
41-70: Well-structured ThreadWrapper component.The new ThreadWrapper component is well-implemented with proper TypeScript typing and serves as a clean adapter between the Thread component and ThreadContextMenu. It abstracts the context menu implementation details away from the Thread component, making the code more modular and maintainable.
94-95: Extracted folder parameter for context-aware actions.Properly using the useParams hook to extract the folder parameter, which is then used to determine folder types for contextual actions.
111-114: Clear folder type logic.The boolean flags for folder types create a clear, readable way to determine the current folder context, which is then used to provide appropriate actions in the context menu.
162-334: Improved component structure with content variable.Extracting the JSX content into a variable improves readability and allows for wrapping it in the ThreadWrapper component. This is a clean refactoring approach.
335-346: Well-implemented wrapper integration.The ThreadWrapper is properly integrated with the necessary props passed down. The refreshCallback using mutate() ensures the UI updates after thread state changes. This implementation follows best practices for component composition.
apps/mail/components/ui/context-menu.tsx (12)
1-8: Well-structured imports for the context menu component.Clean import structure with proper separation of Radix UI primitives, React, and utility functions.
9-20: Proper component aliasing.Good practice to alias the Radix UI primitive components, which makes them more easily consumable by other components in the application.
21-40: Well-implemented ContextMenuSubTrigger.The component properly uses forwardRef and includes an inset prop for flexible styling. The ChevronRight icon provides visual indication of a submenu, enhancing usability.
42-55: Well-structured ContextMenuSubContent component.Clean implementation with proper ref forwarding and styled using the cn utility function. Appropriate animations are included for state transitions.
57-72: ContextMenuContent with proper portal implementation.Using ContextMenuPrimitive.Portal ensures the menu content is rendered at the root of the DOM, preventing z-index and overflow issues. The animations provide a polished user experience.
74-90: Flexible ContextMenuItem with inset option.The inset prop provides additional styling flexibility for menu items, useful for creating hierarchical or aligned menu structures.
92-113: Accessible ContextMenuCheckboxItem.Well-implemented checkbox item with proper accessibility features, including a visual indicator for the checked state and appropriate styling for disabled states.
115-135: Properly styled ContextMenuRadioItem.The radio item implementation includes appropriate styling and a circular indicator, maintaining consistency with standard radio input UI patterns.
137-149: Semantically correct ContextMenuLabel.The label component is styled appropriately with semibold font weight to distinguish it from regular menu items, enhancing the menu's information hierarchy.
151-161: Clean separator implementation.Simple yet effective separator component that provides visual grouping of menu items.
163-171: Useful ContextMenuShortcut component.This component provides a consistent way to display keyboard shortcuts in the menu, enhancing usability for power users.
173-189: Comprehensive exports.All components are properly exported, making them easily importable by other components that need context menu functionality.
|
|
||
| export const toggleStar = async ({ ids }: { ids: string[] }) => { | ||
| try { | ||
| const driver = await getActiveDriver(); | ||
| const { threadIds } = driver.normalizeIds(ids); | ||
|
|
||
| if (threadIds.length && threadIds[0]) { | ||
| const thread = await driver.get(threadIds[0]); | ||
| if (!thread?.[0]) { | ||
| return { success: false, error: 'Thread not found' }; | ||
| } | ||
|
|
||
| const isStarred = thread[0].tags?.includes('STARRED') ?? false; | ||
|
|
||
| await driver.modifyLabels(threadIds, { | ||
| addLabels: isStarred ? [] : ['STARRED'], | ||
| removeLabels: isStarred ? ['STARRED'] : [], | ||
| }); | ||
|
|
||
| return { success: true }; | ||
| } | ||
|
|
||
| return { success: false, error: 'No thread IDs provided' }; | ||
| } catch (error) { | ||
| if (FatalErrors.includes((error as Error).message)) await deleteActiveConnection(); | ||
| console.error('Error toggling star:', error); | ||
| throw error; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address multi-thread behavior for the toggleStar function.
Currently, the code checks the star status of only the first thread (line 121), while the label modification (line 128) targets all threads in threadIds. This can lead to inconsistencies if multiple threads have different star states. Consider iterating over each thread, determining its existing star state individually, and then applying the label changes accordingly. For example:
-export const toggleStar = async ({ ids }: { ids: string[] }) => {
- try {
- const driver = await getActiveDriver();
- const { threadIds } = driver.normalizeIds(ids);
- if (threadIds.length && threadIds[0]) {
- const thread = await driver.get(threadIds[0]);
- if (!thread?.[0]) {
- return { success: false, error: 'Thread not found' };
- }
- const isStarred = thread[0].tags?.includes('STARRED') ?? false;
- await driver.modifyLabels(threadIds, {
- addLabels: isStarred ? [] : ['STARRED'],
- removeLabels: isStarred ? ['STARRED'] : [],
- });
- return { success: true };
- }
- return { success: false, error: 'No thread IDs provided' };
- } catch (error) {
- if (FatalErrors.includes((error as Error).message)) await deleteActiveConnection();
- console.error('Error toggling star:', error);
- throw error;
- }
-};
+export const toggleStar = async ({ ids }: { ids: string[] }) => {
+ try {
+ const driver = await getActiveDriver();
+ const { threadIds } = driver.normalizeIds(ids);
+ if (!threadIds.length) {
+ return { success: false, error: 'No thread IDs provided' };
+ }
+
+ // Process each thread individually.
+ for (const tId of threadIds) {
+ const thread = await driver.get(tId);
+ if (!thread?.[0]) {
+ continue;
+ }
+ const isStarred = thread[0].tags?.includes('STARRED') ?? false;
+ await driver.modifyLabels([tId], {
+ addLabels: isStarred ? [] : ['STARRED'],
+ removeLabels: isStarred ? ['STARRED'] : [],
+ });
+ }
+ return { success: true };
+ } catch (error) {
+ if (FatalErrors.includes((error as Error).message)) await deleteActiveConnection();
+ console.error('Error toggling star:', 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 const toggleStar = async ({ ids }: { ids: string[] }) => { | |
| try { | |
| const driver = await getActiveDriver(); | |
| const { threadIds } = driver.normalizeIds(ids); | |
| if (threadIds.length && threadIds[0]) { | |
| const thread = await driver.get(threadIds[0]); | |
| if (!thread?.[0]) { | |
| return { success: false, error: 'Thread not found' }; | |
| } | |
| const isStarred = thread[0].tags?.includes('STARRED') ?? false; | |
| await driver.modifyLabels(threadIds, { | |
| addLabels: isStarred ? [] : ['STARRED'], | |
| removeLabels: isStarred ? ['STARRED'] : [], | |
| }); | |
| return { success: true }; | |
| } | |
| return { success: false, error: 'No thread IDs provided' }; | |
| } catch (error) { | |
| if (FatalErrors.includes((error as Error).message)) await deleteActiveConnection(); | |
| console.error('Error toggling star:', error); | |
| throw error; | |
| } | |
| }; | |
| export const toggleStar = async ({ ids }: { ids: string[] }) => { | |
| try { | |
| const driver = await getActiveDriver(); | |
| const { threadIds } = driver.normalizeIds(ids); | |
| if (!threadIds.length) { | |
| return { success: false, error: 'No thread IDs provided' }; | |
| } | |
| // Process each thread individually. | |
| for (const tId of threadIds) { | |
| const thread = await driver.get(tId); | |
| if (!thread?.[0]) { | |
| continue; | |
| } | |
| const isStarred = thread[0].tags?.includes('STARRED') ?? false; | |
| await driver.modifyLabels([tId], { | |
| addLabels: isStarred ? [] : ['STARRED'], | |
| removeLabels: isStarred ? ['STARRED'] : [], | |
| }); | |
| } | |
| return { success: true }; | |
| } catch (error) { | |
| if (FatalErrors.includes((error as Error).message)) await deleteActiveConnection(); | |
| console.error('Error toggling star:', error); | |
| throw error; | |
| } | |
| }; |
| const handleFavorites = () => { | ||
| const targets = mail.bulkSelected.length ? mail.bulkSelected : [threadId]; | ||
| const promise = toggleStar({ ids: targets }).then(() => { | ||
| setMail(prev => ({ ...prev, bulkSelected: [] })); | ||
| return mutate(); | ||
| }); | ||
|
|
||
| toast.promise(promise, { | ||
| loading: isStarred ? t('common.actions.removingFromFavorites') : t('common.actions.addingToFavorites'), | ||
| success: isStarred ? t('common.actions.removedFromFavorites') : t('common.actions.addedToFavorites'), | ||
| error: t('common.actions.failedToModifyFavorites'), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align all star toggles with partial or unified approach.
This block unconditionally calls toggleStar for all selected threads, but toggleStar (in its current form) only checks the first thread’s state. If you intend to star/unstar all threads consistently regardless of their individual states, consider clearly documenting this choice. Otherwise, refine the logic to handle each thread’s current star status.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/actions/mail.ts (1)
115-154: The toggleStar implementation looks good but has an unnecessary continue statement.The function correctly handles multiple threads by checking each thread's star status individually before applying a consistent action across all threads. This is a robust approach that addresses the concerns raised in previous reviews.
Remove the unnecessary continue statement in the catch block as it's the last statement in the loop:
} catch (error) { - continue; }🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/actions/mail.ts(1 hunks)apps/mail/app/api/driver/google.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/actions/mail.ts (1)
apps/mail/app/api/driver/google.ts (1)
driver(81-589)
🪛 Biome (1.9.4)
apps/mail/actions/mail.ts
[error] 138-138: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (5)
apps/mail/actions/mail.ts (2)
124-141: Smart thread state detection logic.This implementation efficiently determines if all threads are starred by setting
allStarredto false once any unstarred thread is found, then breaking the loop. This optimizes performance by avoiding unnecessary checks once the state is known.🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
141-147:Details
❓ Verification inconclusive
Verify the shouldStar logic is correct.
The current logic sets
shouldStar = !anyValid || !allStarred, which means:
- If no valid threads were found, star them all
- If any thread isn't starred, star them all
- If all threads are already starred, unstar them all
This logic will work for most cases, but consider if this behavior matches the intended UX.
🏁 Script executed:
#!/bin/bash # Check if this function is used in any UI components to verify the expected behavior rg --type ts "toggleStar" --glob "apps/mail/components/**/*.tsx" -A 5 -B 5Length of output: 1798
Action Required: Confirm that the toggle logic in
shouldStarmeets UX expectationsThe code currently calculates:
shouldStar = !anyValid || !allStarred, meaning:
- If no valid threads are present, star them all.
- If at least one valid thread is not starred, star them all.
- Only if all valid threads are already starred will it unstar them all.
We verified that the
toggleStarfunction (invoked inapps/mail/components/context/thread-context.tsx) uses this logic when handling favorites. Please take a moment to confirm that having the toggle star all threads when even one is unstarred is the intended behavior for our UX.apps/mail/app/api/driver/google.ts (3)
313-313: Improved logging for better debugging.Enhanced log message to clearly indicate when a thread is being fetched.
317-325: Commented-out debugging code could be useful.This commented-out block provides a way to debug label-related issues by tracking both thread and message-level labels. Consider keeping this available but disabled for troubleshooting.
472-492: Robust implementation for modifying labels at the message level.The refactored
modifyLabelsmethod now:
- Works with multiple thread IDs
- Fetches all messages within each thread
- Applies label changes at the message level rather than thread level
This is a significant improvement that ensures labels are properly applied to all messages in a thread, which is essential for features like starring/unstarring.
2 participants
Summary by CodeRabbit