Conversation
…ki0/Mail-0 into feature/addFavourite
|
@SakataGintoki0 is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces formatting improvements and refactoring in the mail driver's API files, including changes to the Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 1
🧹 Nitpick comments (2)
apps/mail/locales/ar.json (1)
82-83: Arabic Localization Review for 'favourites':
The entry"favourites": "Favourites"has been added as per the current implementation. Given that the other language files are localized, you may want to consider whether a proper Arabic translation (for example, "المفضلة") is more suitable to enhance the localization consistency.apps/mail/components/mail/thread-display.tsx (1)
565-570: Star functionality duplicated in secondary location.The Star button appears in two locations in the UI, which is unnecessary duplication. Consider removing one of the instances to maintain UI clarity and consistency.
Choose one location for the Star button based on user experience priority:
- Keep it in the loading state UI (lines 317-323) for consistency
- Or keep it in the loaded state UI (lines 565-570) where it's actually functional
- Remove the other instance to avoid confusion
-<ThreadActionButton - icon={Star} - label={'Add to favourites'} - onClick={handleFavourites} - className="relative top-0.5" -/>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/mail/app/api/driver/google.ts(1 hunks)apps/mail/app/api/driver/types.ts(1 hunks)apps/mail/components/mail/mail-list.tsx(2 hunks)apps/mail/components/mail/mail.tsx(3 hunks)apps/mail/components/mail/thread-display.tsx(6 hunks)apps/mail/locales/ar.json(1 hunks)apps/mail/locales/ca.json(1 hunks)apps/mail/locales/de.json(1 hunks)apps/mail/locales/en.json(1 hunks)apps/mail/locales/es.json(1 hunks)apps/mail/locales/fr.json(1 hunks)apps/mail/locales/hi.json(1 hunks)apps/mail/locales/ja.json(1 hunks)apps/mail/locales/ko.json(1 hunks)apps/mail/locales/pl.json(1 hunks)apps/mail/locales/pt.json(1 hunks)apps/mail/locales/ru.json(1 hunks)apps/mail/locales/tr.json(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
apps/mail/components/mail/mail-list.tsx (1)
apps/mail/lib/utils.ts (1) (1)
cn(47-47)
apps/mail/components/mail/thread-display.tsx (2)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(108-119)useThreads(68-106)apps/mail/actions/mail.ts (1) (1)
modifyLabels(80-113)
🔇 Additional comments (23)
apps/mail/locales/hi.json (1)
76-84: New Category Addition in Hindi Locale:
The"mailCategories"section now includes the new key"favourites"with the value"पसंदीदा". The translation appears appropriate and consistent with the other categories.apps/mail/locales/en.json (1)
76-84: Consistent Addition in English Locale:
The addition of the"favourites": "Favourites"entry in the"mailCategories"section is correctly added and matches the changes made across other locale files.apps/mail/locales/pl.json (1)
76-84: New Category Addition in Polish Locale:
The"mailCategories"section now correctly includes the"favourites": "Favourites"key-value pair, ensuring consistency with other localizations.apps/mail/locales/pt.json (1)
76-84: New Category Addition in Portuguese Locale:
The new"favourites": "Favoritos"entry under"mailCategories"has been added correctly. The translation fits well alongside the existing categories.apps/mail/locales/es.json (1)
82-83: New 'favourites' Entry in Spanish Localization:
The addition of"favourites": "Favoritos"to themailCategoriessection is clear and consistent with the purpose of categorizing favorite emails. Ensure that this translation aligns with other UI text and style guidelines.apps/mail/locales/ja.json (1)
82-83: New 'favourites' Entry in Japanese Localization:
The new key"favourites": "お気に入り"is correctly added after the"social"entry. This value is appropriate for the Japanese context. Please verify that the change is reflected consistently across the UI components that reference mail categories.apps/mail/locales/de.json (1)
82-83: New 'favourites' Localization Key in German:
The new key"favourites": "Favoriten"is correctly inserted in themailCategoriessection and fits well with the existing terms. The translation appears accurate.apps/mail/locales/ko.json (1)
82-83: New 'favourites' Entry in Korean Localization:
The addition"favourites": "즐겨찾기"is well integrated into themailCategoriessection. This translation is in line with common Korean usage and appears consistent with similar entries in other languages.apps/mail/locales/fr.json (1)
82-83: Proper implementation of localizations for the new favourites feature.The localization for the new "favourites" category is correctly added with the appropriate French translation. The comma on the previous line is also properly added to maintain valid JSON syntax.
apps/mail/locales/ca.json (1)
82-83: Properly implemented Catalan localization for the favourites feature.The localization for the new "favourites" category is correctly added with the appropriate Catalan translation ("Preferits"). The comma added to the previous "social" entry maintains valid JSON syntax.
apps/mail/app/api/driver/types.ts (1)
26-26: Interface simplification looks good.The simplified return type for
normalizeIdsmethod removes the redundantnormalizedIdsarray, making the code more concise while maintaining functionality. This change aligns well with the implementation in google.ts.apps/mail/app/api/driver/google.ts (1)
384-396: Streamlined ID normalization implementation.The implementation now correctly matches the updated interface by consolidating all IDs into the
threadIdsarray. This simplifies the logic while maintaining the same functionality, as non-thread IDs are now also included in thethreadIdsarray.apps/mail/components/mail/mail-list.tsx (2)
18-18: Import organization looks good.This small organizational change improves code readability by moving the import statement to a standalone line.
452-455: Formatting improvement enhances readability.The JSX element has been properly reformatted with attributes on separate lines, following consistent code style patterns.
apps/mail/locales/tr.json (1)
2-318: Thorough localization update ensures consistency.The Turkish localization file has been completely reformatted and structured to match other localization files in the project. This ensures consistency across language files and supports the new "favourites" feature.
While reviewing, I noticed that these sections in the file could benefit from Turkish translations for the new feature:
- Lines 198-208: Related to mail actions including "Add Star"
- Lines 565-568: Section for favourite functionality in thread display
Consider requesting translations for these sections if they're missing.
apps/mail/components/mail/mail.tsx (3)
5-5: Star icon import added for favourites functionality.The Star icon from lucide-react has been properly imported to support the new favourites feature.
19-19: Fixed MessageKey import.Changed MessageKey from a commented import to an active import statement, which is a good practice and ensures proper type checking.
436-442: New favourites category implementation looks great.The favourites category has been properly added to the mail categories list with appropriate:
- Localization key: 'common.mailCategories.favourites'
- Search filter: 'is:starred'
- Icon: Star component with appropriate styling
- Color scheme: pink theme that fits well with the existing category styles
This implementation aligns well with the PR objective to allow users to star their favorite emails.
apps/mail/components/mail/thread-display.tsx (5)
10-10: Star icon import added correctly.The Star icon has been imported from lucide-react to support the favourites feature implementation.
19-19: Updated imports to support favourites functionality.The import statement has been correctly updated to include both
useThreadanduseThreadshooks to support the favourites feature.
203-204: Thread data management properly implemented.The code now correctly destructures
mutatefromuseThreadand adds a newmutateThreadsfromuseThreads('STARRED'), allowing for proper data refresh when favourites are toggled.
251-263: Well-implemented favourites toggle functionality.The
handleFavouritesfunction provides a clean implementation for toggling the "STARRED" label on emails:
- Properly checks if email is already starred
- Uses the
modifyLabelsaction to add/remove the "STARRED" label- Provides user feedback via toast notifications
- Updates both the thread and threads data with
mutateandmutateThreadsThis implementation supports the PR objective of allowing users to star their favorite emails.
317-323: Star functionality added to primary action bar.The Star button has been appropriately added to the primary action bar for quick access, following the same pattern as other action buttons.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/mail/components/mail/thread-display.tsx (2)
251-263: Consider improving the handleFavourites function with dynamic label text.The function logic is correct, but the label text could be more dynamic. Currently, the button always shows "Add to favourites" even when the action would be removing from favorites.
const handleFavourites = async () => { if (!emailData) return; if (emailData[0]?.tags.includes('STARRED')) { await modifyLabels({ threadId: [emailData[0]?.threadId || ''], removeLabels: ['STARRED'] }); toast('Removed from favourites.'); } else { await modifyLabels({ threadId: [emailData[0]?.threadId || ''], addLabels: ['STARRED'] }); toast('Added to favourites.'); } await mutate(); await mutateThreads(); };
565-570: Consider using localized text for favourites label and dynamic state.Similar to the previous instance, this button should use localized text and indicate the current state (add/remove).
<ThreadActionButton icon={Star} - label={'Add to favourites'} + label={emailData && emailData[0]?.tags.includes('STARRED') ? t('common.threadDisplay.removeFromFavourites') : t('common.threadDisplay.addToFavourites')} onClick={handleFavourites} className="relative top-0.5" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/app/api/driver/google.ts(1 hunks)apps/mail/components/mail/thread-display.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mail/app/api/driver/google.ts
🔇 Additional comments (4)
apps/mail/components/mail/thread-display.tsx (4)
10-10: LGTM! Added the Star icon import for the favourites feature.The Star icon is correctly imported from lucide-react and will be used for the new favourites feature buttons.
19-19: LGTM! Updated import to include useThreads hook.Correctly updated the import to include the useThreads hook which is needed for the new favourites feature.
26-26: LGTM! Added modifyLabels import.The modifyLabels function is properly imported and will be used for toggling the STARRED label on emails.
202-203: LGTM! Added mutate functions for data refresh.Good job adding the mutate functions to refresh thread data after modifying labels. Including both the individual thread mutate and the starred threads collection mutate ensures consistent UI state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/mail/app/api/driver/google.ts (1)
104-146: Consider extracting headers lookup to a helper functionThe repeated pattern of finding header values in the payload could be extracted to a helper function to improve readability and maintainability.
+ const getHeaderValue = ( + payload: gmail_v1.Schema$MessagePart | undefined, + headerName: string, + defaultValue: string = '' + ): string => { + return payload?.headers?.find( + (h) => h.name?.toLowerCase() === headerName.toLowerCase() + )?.value || defaultValue; + }; const parse = ({ id, threadId, snippet, labelIds, payload, }: gmail_v1.Schema$Message): Omit< ParsedMessage, 'body' | 'processedHtml' | 'blobUrl' | 'totalReplies' > => { - const receivedOn = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'date')?.value || 'Failed'; - const sender = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'from')?.value || 'Failed'; - const subject = payload?.headers?.find((h) => h.name?.toLowerCase() === 'subject')?.value || ''; - const references = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'references')?.value || ''; - const inReplyTo = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'in-reply-to')?.value || ''; - const messageId = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'message-id')?.value || ''; - const listUnsubscribe = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'list-unsubscribe')?.value || - undefined; - const listUnsubscribePost = - payload?.headers?.find((h) => h.name?.toLowerCase() === 'list-unsubscribe-post')?.value || - undefined; + const receivedOn = getHeaderValue(payload, 'date', 'Failed'); + const sender = getHeaderValue(payload, 'from', 'Failed'); + const subject = getHeaderValue(payload, 'subject', ''); + const references = getHeaderValue(payload, 'references', ''); + const inReplyTo = getHeaderValue(payload, 'in-reply-to', ''); + const messageId = getHeaderValue(payload, 'message-id', ''); + const listUnsubscribe = getHeaderValue(payload, 'list-unsubscribe') || undefined; + const listUnsubscribePost = getHeaderValue(payload, 'list-unsubscribe-post') || undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/app/api/driver/google.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/app/api/driver/google.ts (3)
apps/mail/app/api/driver/types.ts (2) (2)
IConfig(33-38)MailManager(3-31)apps/mail/types/index.ts (1) (1)
ParsedMessage(32-55)apps/mail/lib/email-utils.ts (1) (1)
parseFrom(140-160)
🪛 Biome (1.9.4)
apps/mail/app/api/driver/google.ts
[error] 238-295: This property value named list is later overwritten by an object member with the same name.
Overwritten with this value.
If an object property with the same name is defined multiple times (except when combining a getter with a setter), only the last definition makes it into the object and previous definitions are ignored.
Unsafe fix: Remove this property value named list
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (3)
apps/mail/app/api/driver/google.ts (3)
442-448: LGTM: This change looks good and aligns with your feature updatesThe
normalizeIdsmethod has been simplified to only returnthreadIds. This aligns with the changes in theMailManagerinterface type definition and supports the new favorites functionality.
449-458: New module supports toggling favoritesThis
modifyLabelsmethod enables the new favorites feature by allowing labels like 'STARRED' to be added or removed from email threads. The implementation is clean and follows the Gmail API's pattern.
416-417:Details
❓ Verification inconclusive
Consider restoring blob URL for HTML rendering
There's a commented line that would create a blob URL for the decoded HTML content, but it's not being used. If this was intentionally removed, consider adding a comment explaining why, or if it was accidental, consider restoring it.
🏁 Script executed:
#!/bin/bash # Description: Check if the blobUrl is used elsewhere in the codebase # Search for blobUrl usage rg "blobUrl" -A 3 -B 3Length of output: 2695
Action: Clarify or Restore Blob URL Generation for HTML Rendering
The file
apps/mail/app/api/driver/google.tscurrently setsblobUrlto an empty string instead of generating a blob URL for the decoded HTML content. We noticed that a data URI creation (usingencodeURIComponent(decodedBody)) was originally implemented but has since been commented out. Since theblobUrlproperty is later used (e.g., for logging its existence and length), please confirm whether disabling blob URL generation was intentional. If so, adding an inline comment explaining the rationale would help future maintainers. Otherwise, consider restoring the original implementation to support HTML rendering.
- Location:
apps/mail/app/api/driver/google.tsat lines 416–417- Suggestion:
- If intended, document why the blob URL generation is omitted.
- If accidental, reinstate the blob URL code (the data URI creation) to enable proper HTML content handling.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/mail/components/mail/thread-display.tsx (2)
286-291: 🛠️ Refactor suggestionUse localized text and dynamic icon state for the favourites button.
The button should use localized text via the translation system and visually indicate whether the email is already in favourites.
<ThreadActionButton - icon={Star} + icon={emailData && emailData[0]?.tags.includes('STARRED') ? StarFilled : Star} - label={'Add to favourites'} + label={emailData && emailData[0]?.tags.includes('STARRED') + ? t('common.threadDisplay.removeFromFavourites') + : t('common.threadDisplay.addToFavourites')} onClick={handleFavourites} className="relative top-0.5" />You'll need to import the filled star icon:
import { ... Star, + StarFilled, } from 'lucide-react';
382-387: 🛠️ Refactor suggestionUse localized text and dynamic icon state for the favourites button.
Same issue as in the previous button instance - use localized text and show visual feedback about the current state.
<ThreadActionButton - icon={Star} + icon={emailData && emailData[0]?.tags.includes('STARRED') ? StarFilled : Star} - label={'Add to favourites'} + label={emailData && emailData[0]?.tags.includes('STARRED') + ? t('common.threadDisplay.removeFromFavourites') + : t('common.threadDisplay.addToFavourites')} onClick={handleFavourites} className="relative top-0.5" />
🧹 Nitpick comments (3)
apps/mail/components/mail/thread-display.tsx (3)
15-15: Remove duplicate imports.The imports on line 15 (
ArchiveX, Forward, ReplyAll) are already imported in lines 2-11, creating unnecessary duplication.-import { ArchiveX, Forward, ReplyAll } from 'lucide-react';🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Shouldn't redeclare 'ArchiveX'. Consider to delete it or rename it.
'ArchiveX' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'Forward'. Consider to delete it or rename it.
'Forward' is defined here:
(lint/suspicious/noRedeclare)
[error] 15-15: Shouldn't redeclare 'ReplyAll'. Consider to delete it or rename it.
'ReplyAll' is defined here:
(lint/suspicious/noRedeclare)
220-232: Optimize the handleFavourites function.The function has several areas for improvement:
- Consider using
emailData[0]?.threadId- Use
Promise.all()for parallel execution of mutate functions- Implement
toast.promisefor better user feedbackconst handleFavourites = async () => { if (!emailData) return; if (emailData[0]?.tags.includes('STARRED')) { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], removeLabels: ['STARRED'] }); - toast('Removed from favourites.'); + await modifyLabels({ threadId: [mail], removeLabels: ['STARRED'] }); + toast.promise( + modifyLabels({ threadId: [mail], removeLabels: ['STARRED'] }), + { + loading: 'Removing from favourites...', + success: 'Removed from favourites.', + error: 'Failed to remove from favourites.' + } + ); } else { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], addLabels: ['STARRED'] }); - toast('Added to favourites.'); + await modifyLabels({ threadId: [mail], addLabels: ['STARRED'] }); + toast.promise( + modifyLabels({ threadId: [mail], addLabels: ['STARRED'] }), + { + loading: 'Adding to favourites...', + success: 'Added to favourites.', + error: 'Failed to add to favourites.' + } + ); } - await mutate(); - await mutateThreads(); + await Promise.all([mutate(), mutateThreads()]); };
26-26: Remove duplicate import of useThread.The
useThreadhook is already imported on line 20, creating an unnecessary duplicate import.-import { useThread } from '@/hooks/use-threads';🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Shouldn't redeclare 'useThread'. Consider to delete it or rename it.
'useThread' is defined here:
(lint/suspicious/noRedeclare)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/mail/app/api/driver/google.ts(1 hunks)apps/mail/components/mail/thread-display.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/mail/app/api/driver/google.ts
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/components/mail/thread-display.tsx (2)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(108-119)useThreads(68-106)apps/mail/actions/mail.ts (1) (1)
modifyLabels(80-113)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/mail/components/mail/thread-display.tsx (1)
277-280: 🛠️ Refactor suggestionAdd localization and dynamic icon state for favourites button.
The favourites button needs two improvements:
- Use the translation system (t function) for the label instead of hardcoded text
- Modify the icon or its state based on whether the thread is already in favourites
<ThreadActionButton - icon={Star} + icon={emailData && emailData[0]?.tags.includes('STARRED') ? StarFilled : Star} - label={'Add to favourites'} + label={emailData && emailData[0]?.tags.includes('STARRED') + ? t('common.threadDisplay.removeFromFavourites') + : t('common.threadDisplay.addToFavourites')} onClick={handleFavourites} className="relative top-0.5" />The
StarFilledicon would need to be imported from 'lucide-react' or a similar source. Alternatively, you could use CSS to style the Star icon differently when active.
🧹 Nitpick comments (3)
apps/mail/components/mail/thread-display.tsx (3)
210-222: Optimize favourites handling with Promise.all and consider refactoring.While the function logic works correctly, there are some opportunities for optimization:
- Line 213: You could simplify
[emailData[0]?.threadId || '']to just use the- Lines 220-221: Use
Promise.all()for parallel execution of mutations- Consider extracting this function for potential reuse elsewhere
const handleFavourites = async () => { if (!emailData) return; if (emailData[0]?.tags.includes('STARRED')) { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], removeLabels: ['STARRED'] }); + await modifyLabels({ threadId: [mail], removeLabels: ['STARRED'] }); toast('Removed from favourites.'); } else { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], addLabels: ['STARRED'] }); + await modifyLabels({ threadId: [mail], addLabels: ['STARRED'] }); toast('Added to favourites.'); } - await mutate(); - await mutateThreads(); + await Promise.all([mutate(), mutateThreads()]); };
214-214: Use toast.promise for better user feedback.Consider using
toast.promiseto provide better feedback during the asynchronous operations, showing loading, success, and error states.if (emailData[0]?.tags.includes('STARRED')) { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], removeLabels: ['STARRED'] }); - toast('Removed from favourites.'); + await toast.promise( + modifyLabels({ threadId: [emailData[0]?.threadId || ''], removeLabels: ['STARRED'] }), + { + loading: 'Removing from favourites...', + success: 'Removed from favourites.', + error: 'Failed to remove from favourites.' + } + ); } else { - await modifyLabels({ threadId: [emailData[0]?.threadId || ''], addLabels: ['STARRED'] }); - toast('Added to favourites.'); + await toast.promise( + modifyLabels({ threadId: [emailData[0]?.threadId || ''], addLabels: ['STARRED'] }), + { + loading: 'Adding to favourites...', + success: 'Added to favourites.', + error: 'Failed to add to favourites.' + } + ); }Also applies to: 217-217
155-186: Consider adding aria-pressed attribute for accessibility.The
ThreadActionButtoncomponent would benefit from an aria-pressed attribute for toggle buttons like the favourites feature to improve accessibility.function ThreadActionButton({ icon: Icon, label, onClick, disabled = false, className, + pressed, }: { icon: React.ComponentType<React.ComponentPropsWithRef<any>> & { startAnimation?: () => void; stopAnimation?: () => void; }; label: string; onClick?: () => void; disabled?: boolean; className?: string; + pressed?: boolean; }) { const iconRef = useRef<any>(null); return ( <Button disabled={disabled} onClick={onClick} variant="ghost" className={cn('md:h-fit md:px-2', className)} onMouseEnter={() => iconRef.current?.startAnimation?.()} onMouseLeave={() => iconRef.current?.stopAnimation?.()} + aria-pressed={pressed} > <Icon ref={iconRef} className="h-4 w-4" /> <span className="sr-only">{label}</span> </Button> ); }Then implement it on your favourites buttons:
<ThreadActionButton icon={Star} label={t('common.threadDisplay.favourites')} onClick={handleFavourites} pressed={emailData && emailData[0]?.tags.includes('STARRED')} className="relative top-0.5" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/thread-display.tsx(6 hunks)
🔇 Additional comments (4)
apps/mail/components/mail/thread-display.tsx (4)
4-4: Star icon imported correctly for favourites feature.The Star icon is appropriately imported for use in the new favourites functionality.
9-9: Required imports added appropriately.The imports for
useThreads,modifyLabels, andtoastare correctly added to support the favourites functionality.Also applies to: 15-15, 24-24
190-191: Proper hook implementation for data refreshing.The additional
mutatefromuseThreadand newmutateThreadsfromuseThreadsare correctly implemented to enable refreshing thread data after favourites status changes.
373-376: Add localization and dynamic icon state for favourites button (second instance).Same issue as the previous button - needs localization and state-based icon.
<ThreadActionButton - icon={Star} + icon={emailData && emailData[0]?.tags.includes('STARRED') ? StarFilled : Star} - label={'Add to favourites'} + label={emailData && emailData[0]?.tags.includes('STARRED') + ? t('common.threadDisplay.removeFromFavourites') + : t('common.threadDisplay.addToFavourites')} onClick={handleFavourites} className="relative top-0.5" />
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/components/mail/thread-display.tsx (1)
212-227:Details
❓ Verification inconclusive
Well-implemented favorites toggle functionality.
The
handleFavouritesfunction:
- Correctly handles both adding and removing the STARRED label
- Provides user feedback via toast notifications
- Properly updates both the individual thread and threads list
Consider enhancing accessibility by adding an aria-pressed attribute to the button that uses this handler.
🏁 Script executed:
#!/bin/bash # Check if aria-pressed is used in other similar toggle buttons for consistency rg -A 3 "aria-pressed" --type tsxLength of output: 61
Accessibility Enhancement: Add
aria-pressedfor Toggle ConsistencyThe
handleFavouritesfunction inapps/mail/components/mail/thread-display.tsxis well-implemented with clear user feedback and state updates. To further improve accessibility, please add anaria-pressedattribute to the button that triggers this function. This enhancement will support users relying on assistive technologies and maintain consistency with other toggle elements.
- Action: Verify how similar toggle buttons handle the
aria-pressedattribute. (Note: Initial automated search withrgproduced an error due to file type issues. Please re-run a manual inspection or adjust your search using a glob pattern—for example,rg -A 3 "aria-pressed" --glob '*.tsx'—to confirm if other components use it.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/thread-display.tsx(7 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/components/mail/thread-display.tsx (2)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(108-119)useThreads(68-106)apps/mail/actions/mail.ts (1) (1)
modifyLabels(80-113)
🔇 Additional comments (6)
apps/mail/components/mail/thread-display.tsx (6)
4-4: Good addition of Star and StarOff icons.The icons are well-chosen for the favourite functionality, providing clear visual indicators for both states.
9-9: Required imports added correctly.Good job adding all necessary imports for the new favorites functionality:
useThreadsfor managing starred threads listmodifyLabelsfor the backend API calltoastfor user notificationsThis follows the pattern of importing only what's needed.
Also applies to: 15-15, 24-24
190-191: Good use of mutate functions for state management.Extracting both the individual thread's
mutatefunction and the starred threads list'smutateThreadsfunction will allow for proper refreshing of the UI after toggling the favorite status.
193-193: Proper state tracking for starred status.The code correctly tracks the starred status in local state and initializes it from the email data when available.
Also applies to: 203-203
226-226: Good use of Promise.all for concurrent updates.Using
Promise.allfor mutating both the thread and threads list concurrently is a performance best practice, as previously recommended in the PR comments.
281-287: Well-implemented UI for toggling favorites.The button implementation:
- Changes icon based on current state (Star/StarOff)
- Uses translated text via t('common.threadDisplay.favourites')
- Maintains consistent positioning and styling with other action buttons
- Is included in both the loading and loaded states of the UI
The button's appearance properly reflects the current state of the email.
Also applies to: 377-382
| const [isFullscreen, setIsFullscreen] = useState(false); | ||
| const t = useTranslations(); | ||
|
|
||
| console.log('emailData', emailData); |
| useEffect(() => { | ||
| if (emailData?.[0]) { | ||
| setIsMuted(emailData[0].unread ?? false); | ||
| setIsStarred(emailData[0].tags.includes('STARRED')); |
There was a problem hiding this comment.
not needed, use the emailData[0].tags.includes('STARRED') directly
|
|
||
| useEffect(() => { | ||
| if (emailData?.[0]) { | ||
| setIsMuted(emailData[0].unread ?? false); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/components/mail/thread-display.tsx (1)
285-290: Dynamic icon toggle provides good user feedback.The ThreadActionButton implementation correctly:
- Toggles between Star and StarOff icons based on current starred status
- Uses translations for accessibility with the label prop
- Maintains consistent positioning with the className prop
Consider adding an aria-pressed attribute to improve accessibility:
<ThreadActionButton icon={emailData[0]?.tags?.includes('STARRED') ? StarOff : Star} label={t('common.threadDisplay.favourites')} + aria-pressed={emailData[0]?.tags?.includes('STARRED')} onClick={handleFavourites} className="relative top-0.5" />Also applies to: 380-385
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/thread-display.tsx(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/components/mail/thread-display.tsx (2)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(114-125)useThreads(68-112)apps/mail/actions/mail.ts (1) (1)
modifyLabels(80-113)
🔇 Additional comments (5)
apps/mail/components/mail/thread-display.tsx (5)
4-4: Good addition of Star/StarOff icons for favorites feature.The import of both Star and StarOff icons from lucide-react allows for dynamic toggling between the two based on favorite status, providing clear visual feedback to users.
10-10: Properly updated imports to include useThreads hook.Adding the useThreads hook import enables managing and refreshing the favorites list in the UI.
16-16: Correctly added required imports for modifyLabels and toast.These imports are necessary for the favorites functionality - modifyLabels for the API interaction and toast for user feedback.
Also applies to: 25-25
195-196: Good use of mutate functions for state updates.The implementation properly extracts both the individual thread mutate function and the threads list mutate function, allowing for efficient UI updates after modifying favorites status.
214-231: Well-implemented favorites toggle functionality.The handleFavourites function is well-structured with:
- Proper null checking for emailData
- Conditional logic to toggle STARRED status
- Good use of toast.promise for user feedback during async operations
- Appropriate Promise.all to update both the thread and threads list
3 participants
Description
Add to favourties feature is added, which allowes user to star their favourite mails.
Type of Change
Please delete options that are not relevant.
Areas Affected
Please check all that apply:
Testing Done
Describe the tests you've done:
Security Considerations
For changes involving data or authentication:
Checklist
Screenshots/Recordings
Add screenshots or recordings here if applicable.
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit