-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Integrate IMAP and SMTP clients for message sending #14688
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This PR implements IMAP and SMTP client integration for message sending, addressing the feature request to copy workflow-sent emails to the "Sent" folder. The implementation adds support for storing sent messages in IMAP accounts' Sent folders.
Key Changes:
- Added IMAP client integration alongside SMTP for IMAP_SMTP_CALDAV provider
- Implemented parallel client initialization and proper message composition using MailComposer
- Added automatic copying of sent messages to the Sent folder when available
- Simplified Gmail message composition by removing multipart/alternative format
- Removed HTML support from the interface, now only supporting plain text messages
Critical Issues Found:
- Resource leak: SMTP client connections are not properly closed
- Removed HTML support breaks backward compatibility for existing workflows
- Missing error handling for IMAP operations could lead to partial failures
- Potential inconsistency between message formats across different providers
Confidence Score: 2/5
- This PR has critical resource management and backward compatibility issues
- Score reflects resource leaks, breaking changes to HTML support, insufficient error handling for IMAP operations, and potential partial failure scenarios
- messaging-send-message.service.ts requires attention for resource management, error handling, and backward compatibility
Important Files Changed
File Analysis
Filename | Score | Overview |
---|---|---|
packages/twenty-server/src/modules/messaging/message-import-manager/services/messaging-send-message.service.ts | 2/5 | integrates IMAP/SMTP for message sending with Sent folder storage, but removes HTML support and has resource leak issues |
Sequence Diagram
sequenceDiagram
participant W as Workflow
participant MS as MessagingSendMessageService
participant SC as SmtpClient
participant IC as ImapClient
participant GC as GmailClient
participant MC as MicrosoftClient
participant SF as SentFolder
W->>MS: sendMessage(input, connectedAccount)
alt Google Provider
MS->>GC: getGmailClient()
MS->>GC: userinfo.get()
MS->>MS: buildHeaders(fromEmail, fromName)
MS->>GC: users.messages.send(encodedMessage)
else Microsoft Provider
MS->>MC: getMicrosoftClient()
MS->>MC: api('/me/messages').post(message)
MS->>MC: api('/me/messages/{id}/send').post()
else IMAP_SMTP_CALDAV Provider
par Parallel Client Setup
MS->>SC: getSmtpClient()
and
MS->>IC: getClient()
end
MS->>MS: new MailComposer(messageData)
MS->>MS: compile().build() -> messageBuffer
MS->>SC: sendMail(rawMessage)
MS->>IC: list() -> folders
MS->>SF: find(specialUse === '\\Sent')
alt Sent Folder Found
MS->>IC: append(sentFolder.path, messageBuffer)
end
MS->>IC: closeClient()
Note over MS,SC: SMTP client not closed (resource leak)
end
1 file reviewed, 4 comments
to: [sendMessageInput.to], | ||
raw: messageBuffer, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: use object destructuring for better readability
to: [sendMessageInput.to], | |
raw: messageBuffer, | |
}); | |
await smtpClient.sendMail({ | |
from: connectedAccount.handle, | |
to: [sendMessageInput.to], | |
raw: messageBuffer, | |
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JealousGx
Could you please address this last comment?
const sentFolder = await imapClient | ||
.list() | ||
.then((folders) => | ||
folders.find((folder) => folder.specialUse === '\\Sent'), | ||
); | ||
|
||
if (isDefined(sentFolder)) { | ||
await imapClient.append(sentFolder.path, messageBuffer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: if the Sent folder lookup fails (network error, permissions), the message is sent but not saved to Sent folder. add error handling to ensure message is either fully sent and saved, or operation fails completely
const sentFolder = await imapClient | |
.list() | |
.then((folders) => | |
folders.find((folder) => folder.specialUse === '\\Sent'), | |
); | |
if (isDefined(sentFolder)) { | |
await imapClient.append(sentFolder.path, messageBuffer); | |
} | |
const sentFolder = await imapClient | |
.list() | |
.then((folders) => | |
folders.find((folder) => folder.specialUse === '\\Sent'), | |
) | |
.catch((error) => { | |
throw new MessageImportDriverException( | |
`Failed to find Sent folder: ${error.message}`, | |
MessageImportDriverExceptionCode.CLIENT_NOT_AVAILABLE, | |
); | |
}); | |
if (isDefined(sentFolder)) { | |
await imapClient.append(sentFolder.path, messageBuffer) | |
.catch((error) => { | |
throw new MessageImportDriverException( | |
`Failed to save message to Sent folder: ${error.message}`, | |
MessageImportDriverExceptionCode.CLIENT_NOT_AVAILABLE, | |
); | |
}); | |
} |
await imapClient.append(sentFolder.path, messageBuffer); | ||
} | ||
|
||
await this.imapClientProvider.closeClient(imapClient); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: IMAP client is closed but SMTP client is not. this creates resource leaks for SMTP connections
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:59914 This environment will automatically shut down when the PR is closed or after 5 hours. |
raw: messageBuffer, | ||
}); | ||
|
||
const sentFolder = await imapClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not efficient and also doesn't accounts for edge cases that ImapFindSentFolderService
handles
I would suggest you to get the sent folder from messageFolders
table instead, filter by isSentFolder: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually no need to query anything, connectedAccount
already has messageChannel
and messageFolders
you can do something like
const messageChannel = connectedAccount.messageChannels.find(
(channel) => channel.handle === connectedAccount.handle
);
const sentFolder = messageChannel?.messageFolders.find(
(messageFolder) => messageFolder.isSentFolder
);
await this.smtpClientProvider.getSmtpClient(connectedAccount); | ||
const [smtpClient, imapClient] = await Promise.all([ | ||
this.smtpClientProvider.getSmtpClient(connectedAccount), | ||
this.imapClientProvider.getClient(connectedAccount), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imapClientProvider.getClient()
will throw an error if the user doesn't have an IMAP account connected, this assumes user will always have one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's much we can do on the backend in that case. For convenience, I've added logs when we can't get IMAP account.
.list() | ||
.then((folders) => | ||
folders.find((folder) => folder.specialUse === '\\Sent'), | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and call the client conditionally
isDefined(connectedAccount.connectionParameters?.IMAP)
@JealousGx |
Implement functionality to send messages using both IMAP and SMTP clients, allowing for message storage in the Sent folder.
Closes #14379