Conversation
…ki0/Mail-0 into feature/addFavourite
Feature/add favourite
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR removes the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Email UI Components
participant Hook as useThreads Hook
participant API as getMail API
participant Mark as markAsRead Function
UI->>Hook: Call useThreads()
Hook->>API: Fetch email threads
API-->>Hook: Return threads (including unread messages)
Hook->>Mark: Trigger markAsRead(unreadIds) if unread messages exist
Mark-->>Hook: Acknowledge update
Hook-->>UI: Return updated thread list
Possibly related PRs
Suggested reviewers
Poem
🪧 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
🔭 Outside diff range comments (1)
apps/mail/components/mail/thread-display.tsx (1)
224-224:⚠️ Potential issueFix missing
mutatefunction referenceThere's a call to
mutate()which isn't defined after the refactoring. You need to destructure it from theuseThreadhook.-const { data: emailData, isLoading } = useThread(); +const { data: emailData, isLoading, mutate } = useThread(); const { mutate: mutateThreads } = useThreads();
🧹 Nitpick comments (3)
apps/mail/types/index.ts (1)
102-102: Consider using a more specific return type thananyThe type signature for the
onClickhandler has been broadened fromPromise<any> | undefinedto justany. While this provides more flexibility and aligns with the refactoring objectives, it reduces type safety. Consider using a more specific return type if possible (e.g.,Promise<void> | voidorunknown).- onClick?: (message: InitialThread) => () => any; + onClick?: (message: InitialThread) => () => Promise<void> | void;apps/mail/hooks/use-threads.ts (2)
45-58: Improve type safety in the fetchThread functionThe implementation of optimistic updates for marking unread messages as read is a good enhancement. However, the use of
anytype for both the callback parameter and args array reduces type safety.Consider using more specific types:
-const fetchThread = (cb: any) => async (args: any[]) => { +const fetchThread = (cb?: () => void) => async (args: [string, string, string]) => { const [_, id] = args; try { return await getMail({ id }).then((response) => { if (response) { const unreadMessages = response.filter(e=>e.unread).map(e=>e.id) if (unreadMessages.length) { markAsRead({ids: unreadMessages}).then(()=>{ - if (cb && typeof cb === 'function') cb() + if (cb) cb() }); } return response } });
137-137: Consider removing the 'as any' castThe code is passing the
mutateThreadsfunction tofetchThread, but relies on a type cast toany. With proper typing of thefetchThreadfunction (as suggested earlier), this cast would be unnecessary.- fetchThread(mutateThreads) as any, + fetchThread(mutateThreads),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/mail/app/(routes)/mail/page.tsx(0 hunks)apps/mail/components/context/thread-context.tsx(1 hunks)apps/mail/components/draft/drafts-list.tsx(1 hunks)apps/mail/components/mail/mail-list.tsx(4 hunks)apps/mail/components/mail/mail.tsx(1 hunks)apps/mail/components/mail/thread-display.tsx(1 hunks)apps/mail/components/mail/thread-subject.tsx(1 hunks)apps/mail/hooks/use-threads.ts(4 hunks)apps/mail/lib/email-utils.client.tsx(1 hunks)apps/mail/types/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/mail/app/(routes)/mail/page.tsx
🧰 Additional context used
🧬 Code Definitions (5)
apps/mail/components/mail/thread-display.tsx (1)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(129-143)useThreads(81-127)
apps/mail/components/mail/mail.tsx (1)
apps/mail/hooks/use-threads.ts (1) (1)
useThreads(81-127)
apps/mail/components/mail/mail-list.tsx (2)
apps/mail/lib/email-utils.client.tsx (1) (1)
highlightText(59-77)apps/mail/hooks/use-threads.ts (1) (1)
useThreads(81-127)
apps/mail/components/context/thread-context.tsx (1)
apps/mail/hooks/use-threads.ts (1) (1)
useThreads(81-127)
apps/mail/hooks/use-threads.ts (3)
apps/mail/actions/mail.ts (2) (2)
getMail(31-43)markAsRead(45-55)apps/mail/lib/utils.ts (1) (1)
defaultPageSize(139-139)apps/mail/types/index.ts (1) (1)
ParsedMessage(37-60)
🔇 Additional comments (16)
apps/mail/lib/email-utils.client.tsx (1)
59-77: Good extraction of the highlightText function to a utility fileThe implementation is clean and handles edge cases well. This refactoring improves code organization by centralizing the text highlighting functionality that was previously duplicated across components.
apps/mail/components/mail/thread-subject.tsx (1)
51-51: Good improvement for UI consistencyTrimming whitespace from the subject ensures a cleaner display without leading or trailing spaces. This small change improves the user experience by presenting content more consistently.
apps/mail/components/draft/drafts-list.tsx (1)
19-19: Good refactoring by centralizing the highlightText function.Moving the highlighting functionality to a dedicated utility file improves code maintainability and ensures consistent behavior across the application.
apps/mail/components/context/thread-context.tsx (2)
68-68: Good simplification of the useThreads hook usage.Removing the parameters from the
mutate()call simplifies the component and aligns with the refactored hook implementation. The hook now internally accesses the necessary context (folder and search parameters) from the URL, which is a cleaner approach.
110-111: Consistent implementation of the simplified mutate call.The simplified
mutate()call is correctly implemented when handling thread moves, maintaining consistency with the updated hook behavior.apps/mail/components/mail/mail.tsx (1)
215-215: Good simplification of the useThreads hook usage.Removing the parameters from the
useThreads()call improves code maintainability by centralizing the logic for retrieving folder and search parameters within the hook itself. This change aligns with the PR objective of refactoring the hook to eliminate parameter requirements.apps/mail/components/mail/mail-list.tsx (6)
15-15: Good refactoring by centralizing the highlightText function.Moving the highlighting functionality to a dedicated utility file improves code maintainability and ensures consistent behavior across the application.
105-106: Improved opacity handling for thread items.The updated condition enhances the visual distinction between different thread states. Now both selected threads and read threads will have reduced opacity, making unread and non-selected threads stand out more clearly.
119-120: Improved visual distinction for unread messages.The conditional font weight now considers both the read status and selection state. This creates a better visual hierarchy by only highlighting truly unread and non-selected messages with bold text.
126-126: Improved unread indicator display logic.The blue dot indicator for unread messages now only appears when the message is both unread and not selected. This creates a cleaner UI by removing redundant visual indicators when a thread is already highlighted through selection.
211-211: Good simplification of the useThreads hook usage.Removing the parameters from the
useThreads()call aligns with the hook's refactored implementation, which now internally accesses folder and search parameters from the URL. This change supports the PR objective of eliminating parameter requirements for the hook.
370-393: Read status is now managed separately from thread selection.The code no longer automatically marks threads as read when clicked. This change aligns with the PR objective of implementing an optimistic update mechanism for the
markAsReadfunctionality, decoupling selection from read status management.apps/mail/hooks/use-threads.ts (4)
3-8: Good refactoring of imports to support new functionalityThe imports have been appropriately updated to support the refactored hooks. Adding
markAsRead,useParams,useSearchParams,useSearchValue, anddefaultPageSizealigns with the PR objective of simplifying thread fetching by relying on URL parameters instead of direct arguments.
15-15: LGTM: Simplified preload functionThe modification to pass
undefinedtofetchThreadaligns with the refactoring approach, as the function now retrieves parameters from context rather than direct arguments.
81-90: Good refactoring to use URL parametersThe
useThreadshook has been properly refactored to use URL parameters instead of function arguments, which aligns with the PR objectives. The implementation correctly extracts the folder from route parameters and search value from the hook, making the code more maintainable.
129-134: Good implementation of thread fetching using URL parametersThe
useThreadhook has been well refactored to use search parameters for the thread ID instead of requiring it as a function parameter. Additionally, using the mutate function fromuseThreadsenables proper state management when threads are updated.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/components/mail/thread-display.tsx (1)
282-282: Consider handling the undefined case differently in icon selection.The current condition
!(emailData) || emailData[0]?.tags?.includes('STARRED')will show the StarOff icon when emailData is undefined. While this prevents a runtime error, it might be misleading to show an "unstar" action when there's no data to act upon.- icon={!(emailData) || emailData[0]?.tags?.includes('STARRED') ? StarOff : Star} + icon={emailData?.[0]?.tags?.includes('STARRED') ? StarOff : Star} + disabled={!emailData}This way, the icon accurately reflects the starred status only when data is available, and the button is disabled when there's no data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/components/mail/thread-display.tsx(3 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/components/mail/thread-display.tsx (1)
apps/mail/hooks/use-threads.ts (2) (2)
useThread(129-143)useThreads(81-127)
🔇 Additional comments (2)
apps/mail/components/mail/thread-display.tsx (2)
189-192: Hooks refactoring successfully implemented!The removal of parameters from the hook calls aligns with the PR objectives. The code now uses the search parameters to get the threadId directly, making the implementation cleaner and more maintainable.
211-211: Good defensive check for thread operations.Adding the null check for both
emailDataandthreadIdis a proper safeguard before performing operations on thread favorites.
|
LGTM |
4 participants
Summary by CodeRabbit
Revert
Refactor
Style
New Features