Including the body of the message when forwarding emails from Mail client. #195
Including the body of the message when forwarding emails from Mail client. #195elie222 merged 1 commit intoelie222:mainfrom
Conversation
|
Someone is attempting to deploy a commit to the Inbox Zero Team on Vercel. A member of the Team first needs to authorize it. |
|
varuntree seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
WalkthroughThe recent modifications enhance the email composition functionality, particularly for replying and forwarding emails. By introducing new fields in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmailPanel
participant ComposeEmailForm
User ->> EmailPanel: Initiates Reply/Forward
EmailPanel ->> EmailPanel: Calls prepareReplyingToEmail or prepareForwardingEmail
EmailPanel ->> ComposeEmailForm: Passes prepared email data
ComposeEmailForm ->> ComposeEmailForm: Concatenates messageText and messageHtml
User ->> ComposeEmailForm: Submits email form
ComposeEmailForm ->> EmailPanel: Sends email data for processing
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- apps/web/app/(app)/compose/ComposeEmailForm.tsx (3 hunks)
- apps/web/components/email-list/EmailPanel.tsx (3 hunks)
Additional context used
Biome
apps/web/components/email-list/EmailPanel.tsx
[error] 59-59: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 100-100: Using the role attribute 'list' on the 'ul' element is redundant, because it is implied by the semantic 'ul' element.
Unsafe fix: Remove the role attribute.
(lint/a11y/noRedundantRoles)
[error] 132-132: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 133-133: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 144-144: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 282-285: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 291-291: Provide a title attribute when using iframe elements.
Screen readers rely on the title set on an iframe to describe the content being displayed.
(lint/a11y/useIframeTitle)
[error] 15-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
apps/web/app/(app)/compose/ComposeEmailForm.tsx
[error] 100-100: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 102-102: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 107-107: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 241-241: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 306-306: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 318-318: Do not use template literals if interpolation and special-character handling are not needed.
Unsafe fix: Replace with string literal
(lint/style/noUnusedTemplateLiteral)
[error] 85-85: This hook does not specify all of its dependencies: props.replyingToEmail?.messageHtml
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 85-85: This hook does not specify all of its dependencies: props.replyingToEmail?.messageText
This dependency is not specified in the hook dependency list.
(lint/correctness/useExhaustiveDependencies)
[error] 87-87: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
[error] 151-151: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
Additional comments not posted (3)
apps/web/components/email-list/EmailPanel.tsx (1)
16-16: Approved the addition of theParsedMessageimport.The import is used effectively in the newly added functions
prepareReplyingToEmailandprepareForwardingEmail.Tools
Biome
[error] 15-16: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the transpilers and avoids loading unnecessary modules.
Safe fix: Use import type.(lint/style/useImportType)
apps/web/app/(app)/compose/ComposeEmailForm.tsx (2)
51-52: Approved the addition ofmessageTextandmessageHtmlfields toReplyingToEmail.These fields are essential for the new feature of including message bodies when replying or forwarding emails.
87-92: Avoid reassigning function parameters and address hook dependency issues.Reassigning parameters can lead to confusing and hard-to-track bugs. Also, ensure that all dependencies are specified in the hook dependency list.
- data = { + const updatedData = { ...data, messageText: data.messageText + props.replyingToEmail?.messageText, messageHtml: (data.messageHtml ?? "") + (props.replyingToEmail?.messageHtml ?? ""), }; + await sendEmail(updatedData); - const res = await postRequest<SendEmailResponse, SendEmailBody>("/api/google/messages/send", data); + const res = await postRequest<SendEmailResponse, SendEmailBody>("/api/google/messages/send", updatedData);Tools
Biome
[error] 87-87: Reassigning a function parameter is confusing.
The parameter is declared here:
Use a local variable instead.
(lint/style/noParameterAssign)
| const prepareReplyingToEmail = (message: ParsedMessage) => ({ | ||
| to: message.headers.from, | ||
| subject: `Re: ${message.headers.subject}`, | ||
| headerMessageId: message.headers["message-id"]!, | ||
| threadId: message.threadId!, | ||
| cc: message.headers.cc, | ||
| references: message.headers.references, | ||
| messageText: "", | ||
| messageHtml: "", | ||
| }); |
There was a problem hiding this comment.
Consider handling potential null values instead of using non-null assertions.
The use of non-null assertions (!) could lead to runtime errors if the properties are actually null.
- headerMessageId: message.headers["message-id"]!,
- threadId: message.threadId!,
+ headerMessageId: message.headers["message-id"] || 'default-message-id',
+ threadId: message.threadId || 'default-thread-id',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.
| const prepareReplyingToEmail = (message: ParsedMessage) => ({ | |
| to: message.headers.from, | |
| subject: `Re: ${message.headers.subject}`, | |
| headerMessageId: message.headers["message-id"]!, | |
| threadId: message.threadId!, | |
| cc: message.headers.cc, | |
| references: message.headers.references, | |
| messageText: "", | |
| messageHtml: "", | |
| }); | |
| const prepareReplyingToEmail = (message: ParsedMessage) => ({ | |
| to: message.headers.from, | |
| subject: `Re: ${message.headers.subject}`, | |
| headerMessageId: message.headers["message-id"] || 'default-message-id', | |
| threadId: message.threadId || 'default-thread-id', | |
| cc: message.headers.cc, | |
| references: message.headers.references, | |
| messageText: "", | |
| messageHtml: "", | |
| }); |
Tools
Biome
[error] 132-132: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
[error] 133-133: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| const prepareForwardingEmail = (message: ParsedMessage) => ({ | ||
| to: "", | ||
| subject: `Fwd: ${message.headers.subject}`, | ||
| headerMessageId: "", | ||
| threadId: message.threadId!, | ||
| cc: "", | ||
| references: "", | ||
| messageText: ` | ||
| \n\n--- Forwarded message --- | ||
| \nFrom: ${message.headers.from} | ||
| \nDate: ${message.headers.date} | ||
| \nSubject: ${message.headers.subject} | ||
| \nTo: ${message.headers.to} | ||
| ${message.textPlain} | ||
| `, | ||
| messageHtml: ` | ||
| <br><br> | ||
| <div style="border-left: 2px solid #ccc; padding-left: 10px; margin: 10px 0;"> | ||
| <p><strong>--- Forwarded message ---</strong></p> | ||
| <p><strong>From:</strong> ${message.headers.from}</p> | ||
| <p><strong>Date:</strong> ${message.headers.date}</p> | ||
| <p><strong>Subject:</strong> ${message.headers.subject}</p> | ||
| <p><strong>To:</strong> ${message.headers.to}</p> | ||
| </div> | ||
| ${message.textHtml} | ||
| `, | ||
| }); |
There was a problem hiding this comment.
Consider handling potential null values instead of using non-null assertions.
Similar to prepareReplyingToEmail, this function also uses non-null assertions that could lead to runtime errors. Additionally, ensure the template literals are necessary for the context, as they introduce potential XSS vulnerabilities.
- threadId: message.threadId!,
+ threadId: message.threadId || 'default-thread-id',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.
| const prepareForwardingEmail = (message: ParsedMessage) => ({ | |
| to: "", | |
| subject: `Fwd: ${message.headers.subject}`, | |
| headerMessageId: "", | |
| threadId: message.threadId!, | |
| cc: "", | |
| references: "", | |
| messageText: ` | |
| \n\n--- Forwarded message --- | |
| \nFrom: ${message.headers.from} | |
| \nDate: ${message.headers.date} | |
| \nSubject: ${message.headers.subject} | |
| \nTo: ${message.headers.to} | |
| ${message.textPlain} | |
| `, | |
| messageHtml: ` | |
| <br><br> | |
| <div style="border-left: 2px solid #ccc; padding-left: 10px; margin: 10px 0;"> | |
| <p><strong>--- Forwarded message ---</strong></p> | |
| <p><strong>From:</strong> ${message.headers.from}</p> | |
| <p><strong>Date:</strong> ${message.headers.date}</p> | |
| <p><strong>Subject:</strong> ${message.headers.subject}</p> | |
| <p><strong>To:</strong> ${message.headers.to}</p> | |
| </div> | |
| ${message.textHtml} | |
| `, | |
| }); | |
| const prepareForwardingEmail = (message: ParsedMessage) => ({ | |
| to: "", | |
| subject: `Fwd: ${message.headers.subject}`, | |
| headerMessageId: "", | |
| threadId: message.threadId || 'default-thread-id', | |
| cc: "", | |
| references: "", | |
| messageText: ` | |
| \n\n--- Forwarded message --- | |
| \nFrom: ${message.headers.from} | |
| \nDate: ${message.headers.date} | |
| \nSubject: ${message.headers.subject} | |
| \nTo: ${message.headers.to} | |
| ${message.textPlain} | |
| `, | |
| messageHtml: ` | |
| <br><br> | |
| <div style="border-left: 2px solid #ccc; padding-left: 10px; margin: 10px 0;"> | |
| <p><strong>--- Forwarded message ---</strong></p> | |
| <p><strong>From:</strong> ${message.headers.from}</p> | |
| <p><strong>Date:</strong> ${message.headers.date}</p> | |
| <p><strong>Subject:</strong> ${message.headers.subject}</p> | |
| <p><strong>To:</strong> ${message.headers.to}</p> | |
| </div> | |
| ${message.textHtml} | |
| `, | |
| }); |
Tools
Biome
[error] 144-144: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
| messageHtml: "", | ||
| }); | ||
|
|
||
| const prepareForwardingEmail = (message: ParsedMessage) => ({ |
There was a problem hiding this comment.
Hey, wondering if we need the same code on the backend when we forward an email?
As there are emails that are forwarded solely via the backend so the logic would be the same thing
There was a problem hiding this comment.
Yeah, we can use the same code, or maybe it even makes more sense if we just use it only in backend.
But, i can't find where exactly we forward the mails solely via backend, can you refer me there?
|
@varuntree could you sign the CLA please (in the checks) |
|
@elie222 Hey, i did signed already, but for some reason its not updated, check all the sources. |
|
Merged. The code where we had something similar already is here: const raw = await createRawMailMessage({
to: options.to,
cc: options.cc,
bcc: options.bcc,
subject: `Fwd: ${message.headers.subject}`,
messageText: `${options.content ?? ""}
---------- Forwarded message ----------
From: ${message.headers.from}
Date: ${message.headers.date}
Subject: ${message.headers.subject}
To: <${message.headers.to}>
${message.textPlain}`,
messageHtml: `<div>${options.content ?? ""}</div>
<div>---------- Forwarded message ----------</div>
<div>From: ${message.headers.from}</div>
<div>Date: ${message.headers.date}</div>
<div>Subject: ${message.headers.subject}</div>
<div>To: <${message.headers.to}></div>
<br>Would be good to share the common code between these files. |
3 participants
As there is no api yet from the google's side to forward a message, we created our own method.
Tried, not to change and add to much of the complexity to the existing code.
Also added the Forwaded meta text for mail body.
Resolves #161
Summary by CodeRabbit
New Features
messageTextandmessageHtmlfor replies and forwards.Refactor
prepareReplyingToEmailandprepareForwardingEmailfunctions.Style