Feature/support for unified mailbox#509
Feature/support for unified mailbox#509dager-mohamed wants to merge 5 commits intoMail-0:stagingfrom
Conversation
|
@dager-mohamed is attempting to deploy a commit to the Zero Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes update error formatting across mail actions and enhance multi-connection support. A new function for deleting active connections is added with improved session and error handling. Mail functions now accept an optional connection identifier and a new unified inbox flag, enabling the fetching and merging of emails from multiple connections. Additional modifications include UI updates with a UnifiedInboxToggle component, revised atoms to handle unified inbox state, and corresponding localization and type enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Toggle as UnifiedInboxToggle
participant Hook as useMail Hook
participant Layout as MailLayout Component
participant API as MailActions (getMails)
User->>Toggle: Clicks unified inbox toggle
Toggle->>Hook: Toggles 'unifiedInbox' state via atom
Hook-->>Layout: Updates config with new unified inbox value
Layout->>API: Calls getMails with unifiedInbox flag & optional connectionId
API-->>Layout: Returns merged emails from multiple connections
sequenceDiagram
participant Caller
participant DAC as deleteActiveConnection Function
participant Session as Session Handler
participant DB as Database/API
Caller->>DAC: Initiate deletion request
DAC->>Session: Retrieve session headers & connectionId
alt Valid session
DAC->>DB: Delete connection and update defaultConnectionId if needed
DB-->>DAC: Acknowledge deletion
DAC->>DAC: Call revalidatePath('/mail')
else Invalid session
DAC-->>Caller: Log error and throw 'Unauthorized, reconnect'
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
dager-mohamed
changed the title
dager-mohamed
changed the title
![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 (5)
apps/mail/package.json (1)
91-91: Version consistency check for new dependencies.You've added two new dependencies with caret (^) versions while most other dependencies use pinned versions. Consider aligning these with the project's versioning strategy for consistency.
- "react-use": "^17.6.0", + "react-use": "17.6.0", - "react-wrap-balancer": "^1.1.1", + "react-wrap-balancer": "1.1.1",Also applies to: 93-93
apps/mail/types/index.ts (1)
81-83: Consider adding JSDoc comments.Adding JSDoc comments to explain the purpose of these new properties would improve code documentation and help other developers understand their usage.
+ /** ID of the connection this thread belongs to */ connectionId?: string; + /** Email address associated with the connection */ connectionEmail?: string; + /** Display name of the connection */ connectionName?: string;apps/mail/hooks/use-threads.ts (2)
57-77: Thread fetching and marking read
IncorporatingconnectionIdintofetchThreadand conditionally marking messages as read works well. Consider adding a catch block or handling errors ifmarkAsReadfails.
138-148: Flattening and sorting threads
Merging threads across pages and sorting by date is straightforward. For large mailboxes, consider potential performance impacts of repeated sorts.apps/mail/actions/mail.ts (1)
45-45: Optional chaining recommendation
You can simplify this null check by using the optional chaining operator, though it is mostly stylistic.- if (result && result.threads) { + if (result?.threads) { result.threads = result.threads.map((thread) => ({🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
apps/mail/actions/connections.ts(6 hunks)apps/mail/actions/mail.ts(2 hunks)apps/mail/actions/utils.ts(1 hunks)apps/mail/components/mail/mail-list.tsx(5 hunks)apps/mail/components/mail/mail.tsx(10 hunks)apps/mail/components/mail/use-mail.ts(1 hunks)apps/mail/hooks/use-threads.ts(4 hunks)apps/mail/locales/en.json(1 hunks)apps/mail/package.json(1 hunks)apps/mail/types/index.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
apps/mail/actions/connections.ts (2)
apps/mail/lib/auth.ts (1)
auth(155-158)apps/mail/actions/utils.ts (1)
deleteActiveConnection(11-29)
apps/mail/actions/utils.ts (2)
apps/mail/actions/connections.ts (1)
deleteActiveConnection(76-93)apps/mail/app/api/driver/google.ts (1)
driver(81-522)
apps/mail/components/mail/mail-list.tsx (5)
apps/mail/components/mail/use-mail.ts (1)
useMail(17-19)apps/mail/hooks/use-threads.ts (1)
preloadThread(15-18)apps/mail/lib/utils.ts (2)
cn(47-47)formatDate(65-106)apps/mail/lib/email-utils.client.tsx (1)
highlightText(59-77)apps/mail/lib/notes-utils.ts (1)
formatDate(72-89)
apps/mail/actions/mail.ts (2)
apps/mail/actions/connections.ts (2)
getConnections(11-42)deleteActiveConnection(76-93)apps/mail/actions/utils.ts (3)
getActiveDriver(31-69)FatalErrors(9-9)deleteActiveConnection(11-29)
apps/mail/hooks/use-threads.ts (5)
apps/mail/actions/mail.ts (3)
getMails(5-80)getMail(82-94)markAsRead(96-112)apps/mail/components/mail/use-mail.ts (1)
useMail(17-19)apps/mail/hooks/use-connections.ts (1)
useConnections(5-39)apps/mail/lib/utils.ts (1)
defaultPageSize(139-139)apps/mail/types/index.ts (1)
ParsedMessage(37-60)
apps/mail/components/mail/mail.tsx (6)
apps/mail/components/ui/drawer.tsx (4)
Drawer(86-86)DrawerContent(91-91)DrawerHeader(92-92)DrawerTitle(94-94)apps/mail/components/ui/tooltip.tsx (1)
Tooltip(59-59)apps/mail/components/mail/use-mail.ts (1)
toggleUnifiedInboxAtom(26-32)apps/mail/hooks/use-connections.ts (1)
useConnections(5-39)apps/mail/components/ui/toggle.tsx (1)
Toggle(44-44)apps/mail/components/ui/resizable.tsx (3)
ResizableHandle(42-42)ResizablePanel(42-42)ResizablePanelGroup(42-42)
🪛 Biome (1.9.4)
apps/mail/actions/mail.ts
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (41)
apps/mail/locales/en.json (1)
215-216: LGTM: Clear localization strings for unified inbox.The newly added strings provide clear explanations to users about what the unified inbox feature does.
apps/mail/actions/connections.ts (1)
17-17: LGTM: Consistent error message formatting.Good job standardizing error message formatting with single quotes throughout the file.
Also applies to: 23-23, 39-40, 50-50, 56-56, 71-72, 101-101, 107-107, 117-117, 129-130
apps/mail/types/index.ts (2)
81-83: LGTM: Added connection properties to InitialThread.These additional properties are well-designed to support the unified inbox feature by storing connection-specific information with each thread.
99-99: LGTM: Consistent string literal formatting.Good job standardizing the string literal formatting with single quotes.
apps/mail/components/mail/use-mail.ts (4)
8-8: Good addition of unifiedInbox to the Config type.This addition properly extends the Config type to support the unified inbox functionality required for this feature.
14-14: Appropriate initialization of unifiedInbox state.Setting the default value to
falseensures the unified inbox is opt-in rather than enabled by default, which is a good UX practice.
21-24: Properly implemented clearBulkSelectionAtom.The atom correctly preserves other state properties while clearing just the bulkSelected array.
26-32: Well-implemented toggle atom for unified inbox.The
toggleUnifiedInboxAtomfollows Jotai patterns correctly with a getter and setter that preserves other state values when toggling the unified inbox state.apps/mail/components/mail/mail-list.tsx (3)
19-19: Restored necessary imports.The reintroduction of the
cnandformatDateimports is appropriate as they're needed for the component's functionality.
82-86: Good fallback implementation for connectionId.The code now correctly handles the possibility of missing connection IDs by using a fallback chain, making the prefetching mechanism more robust.
195-211: Well-implemented unified inbox connection indicator.This section adds a clear visual indicator showing which email account a message belongs to when in unified inbox mode. The implementation is clean with:
- Proper conditional rendering based on the unified inbox state
- Good use of the Badge component for visual distinction
- Helpful tooltip showing the full email address
- Truncation of long connection names with appropriate styling
This enhances user experience by maintaining context when viewing emails from multiple accounts.
apps/mail/actions/utils.ts (2)
31-31: Well-designed optional parameter for connection flexibility.Adding the optional
connectionIdparameter togetActiveDriverprovides the necessary flexibility to support operations on specific connections, which is essential for the unified inbox feature.
40-46: Good implementation of connection ID fallback logic.The code correctly prioritizes the explicitly provided connection ID over the session's connection ID, with appropriate error handling when neither is available.
apps/mail/components/mail/mail.tsx (6)
15-16: Good addition of necessary icons.The
ArchiveandLayersicons are appropriately added to support the new UI elements in the mail component.
37-37: Proper integration with state management.The update correctly imports
toggleUnifiedInboxAtomfrom the use-mail module, maintaining consistency with the state management approach.
218-218: Good atom initialization for toggle functionality.The component correctly initializes the toggle atom for unified inbox functionality.
227-229: Smart conditional display of unified inbox toggle.The code intelligently checks for multiple connections before showing the unified inbox toggle, which is a good UX decision that prevents showing unnecessary UI elements when they're not applicable.
232-260: Well-implemented UnifiedInboxToggle component.The UnifiedInboxToggle component is well-structured with:
- Clear toggling logic that maintains other state properties
- Good use of the Toggle component for interaction
- Helpful tooltip with appropriate translations for better UX
- Proper icon usage for visual clarity
The component encapsulates its functionality well and integrates seamlessly with the existing layout.
381-385: Clean integration of the toggle in the layout.The unified inbox toggle is well-positioned in the UI and its conditional rendering is correctly implemented, ensuring it only appears when there are multiple connections available.
apps/mail/hooks/use-threads.ts (11)
1-1: Client-side directive is appropriate
This ensures the hook operates in a client environment as intended.
7-8: New imports for multi-connection and mail functionality
These imports foruseConnectionsanduseMailare logically consistent with handling multiple mail connections.
20-28: ExtendFetchEmailsTuplefor unified inbox
AddingunifiedInboxto the tuple signature cleanly supports the new unified inbox mode.
31-55: Fetch emails withunifiedInbox
The code passesunifiedInboxtogetMailscorrectly, maintaining robust error handling viatry/catch.
84-84:connectionIdinRawResponse
Including this property inRawResponsealigns with multi-connection support.
89-101: Enhance key generation for SWR
AddingunifiedInboxinto the fetch key ensures correct caching and revalidation for unified inbox.
108-111: Determine a unified inbox
The logic checksmail.unifiedInbox && connections.length > 1, correctly toggling single vs. unified inbox scenarios.
115-125: SWRInfinite parameters
Conditionally building the fetch key using the session and theisUnifiedflag is coherent with the multi-connection design.
168-168: ExposeisUnified
ReturningisUnifiedfrom this hook makes it easy for components to adapt to unified mailbox mode.
176-188: DynamicconnectionIdresolution
AllowingconnectionIdto be taken from URL params, the thread, or session fosters flexible handling of multiple connections.
191-193: SWR key with triple argument
Using[session.user.id, id, connectionIdToUse]as the fetch key helps uniquely identify each thread in SWR.apps/mail/actions/mail.ts (11)
1-3: Server-side actions and connection imports
These imports and the'use server'directive are properly set up for server-side handling of mail actions.
11-13: New parameters for multi-connection
IntroducingconnectionIdandunifiedInboxingetMailsclearly supports the updated mailbox logic.
23-23: Error message standardization
Throwing an error with single quotes is consistent with the chosen style.
27-70: Unified inbox logic
Fetching and merging emails from all connections, appending identifying info, and gracefully handling partial failures is well-designed for multi-account mail.🧰 Tools
🪛 Biome (1.9.4)
[error] 45-45: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
72-74: Single-connection fallback
Reverting to a single-driver approach when only one connection is available avoids unnecessary complexity.
76-79: Fatal error handling
Cleaning up an invalid connection withdeleteActiveConnection()ensures stable behavior when encountering fatal OAuth errors.
82-94: Fetch a specific mail with connection
getMailnow supportsconnectionIdfor multi-connection scenarios, aligning with the rest of the code.
96-112: Mark as read with optional connection
Letting users mark mail read in a chosen or currently active connection provides flexibility for multi-account usage.
114-130: Mark as unread
Mirrors the read logic with consistent handling of multiple connections.
132-141: Count mails by connection
Facilitates retrieving message counts per connection while adhering to the established error handling approach.
143-178: Update labels with optional connection
Applying label modifications on a per-connection basis, along with debug logs, completes multi-connection label management.
|
Does pagination work? |
I am sorry, wdym by pagination? |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/mail/actions/connections.ts (1)
95-95: Consider returning a more detailed response.Other functions in this file return
{ success: true }, but this one returns the result ofrevalidatePath('/mail'). For consistency, consider returning a similar response object with a success indicator, possibly after the revalidation.- return revalidatePath('/mail'); + revalidatePath('/mail'); + return { success: true };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mail/actions/connections.ts(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
apps/mail/actions/connections.ts (2)
apps/mail/lib/auth.ts (1)
auth(155-158)apps/mail/actions/utils.ts (1)
deleteActiveConnection(11-29)
🔇 Additional comments (4)
apps/mail/actions/connections.ts (4)
17-17: Error message formatting is now more consistent.You've standardized error messages to use single quotes across the file, which improves code style consistency.
Also applies to: 23-23, 50-50, 56-56, 108-108, 114-114, 124-124, 136-137
76-100: LGTM! The new function addresses previous feedback about default connection handling.The implementation of
deleteActiveConnectionnow properly updates the user's defaultConnectionId when the active connection being deleted is also the default connection.I noticed a few minor inconsistencies compared to other functions in this file:
- This function uses arrow syntax while others use function declarations
- This function throws the raw error rather than a user-friendly message
- Console error messages use "Server:" prefix while others use "Failed to"
These differences don't affect functionality but might be worth standardizing for better maintainability.
39-40: Error message logging is improved.Appropriate error logging is now in place, with error details being logged to the console while simplified messages are thrown to the client.
Also applies to: 71-72
84-96:Details
❓ Verification inconclusive
Verify behavior when multiple sessions use the same connection.
The function correctly handles the deletion of an active connection, but consider what happens if multiple users/sessions are using the same connection.
🏁 Script executed:
#!/bin/bash # Check if there are any references to connections in session data that might need to be handled rg -A 5 -B 5 "connectionId.*session" --type-add "tsx:*.tsx" --type-add "ts:*.ts" --type ts --type tsxLength of output: 5326
Action Required: Verify Multi-Session Connection Handling
The deletion logic currently targets the connection using the session’s user ID and connection ID. However, given that several parts of the app (e.g. in hooks like
use-stats.ts,use-threads.ts, anduse-notes.tsx) rely onsession.connectionId, please verify that:
- Deleting a connection from one session does not inadvertently impact other sessions if they share the same connection.
- The update that clears the user's
defaultConnectionIdonly affects the intended session without disrupting active sessions still relying on that connection.Please double-check that the overall behavior is safe in scenarios where multiple sessions might reference the same connection.
infinite scroll |
Oh sorry, it doesn't work, lemme fix that. |
|
I just wrote a comment |
2 participants
READ CAREFULLY THEN REMOVE
Remove bullet points that are not relevant.
PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI.
Description
#129
Connection Handling:
apps/mail/actions/connections.ts: Added a new functiondeleteActiveConnectionand updated existing functions to handle connection-specific logic.apps/mail/actions/utils.ts: UpdatedgetActiveDriverto accept an optionalconnectionIdparameter and handle connection-specific logic.Unified Inbox Functionality:
apps/mail/actions/mail.ts: Implemented unified inbox functionality to fetch and merge emails from multiple connections.UI Updates:
apps/mail/components/mail/mail-list.tsx: Updated the mail list component to display connection-specific information and handle unified inbox view.These changes collectively enhance the application's ability to manage multiple connections and provide a more cohesive user experience.
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
Additional Notes
Add any other context about the pull request here.
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
New Features
Style
Localization