[PB-1969]: feat/forward a message#63
Merged
Merged
Conversation
Deploying mail-web with
|
| Latest commit: |
c1efe02
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2cb2df02.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-forward.mail-web-ea0.pages.dev |
|
Warning Review limit reached
More reviews will be available in 3 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughImplements end-to-end email forwarding: a ChangesEmail Forwarding Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/compose-message/index.tsx (1)
60-121: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftForward orchestration logic should be moved out of the component.
The new initialization/hydration/send wiring logic is implemented in
ComposeMessageDialoginstead of a hook (use<Feature>). This makes the component stateful and orchestration-heavy.As per coding guidelines, components should be dumb (render-only), with logic extracted into hooks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/index.tsx` around lines 60 - 121, Extract the forward orchestration logic currently in ComposeMessageDialog into a custom hook (e.g., useComposeMessageOrchestration or useForwardMessageOrchestration). Move the three useEffect blocks, the hydratedForwardAttachmentsRef tracking, and the related state management logic (setInitialValues, the forward mode editor content setup, and the inherited attachments hydration via addInheritedAttachments) into this new hook. Have the hook return the necessary values and callbacks, then update the component to use this hook instead of managing these concerns directly, keeping the component focused on rendering only.Source: Coding guidelines
src/components/compose-message/hooks/useComposeSend.test.ts (1)
43-65: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit tests for inherited-resolution success/failure behavior.
The helper now wires inherited lifecycle callbacks, but this suite should also assert:
- pending inherited attachments are resolved and included in payload, and
- failure path shows error and aborts
sendEmail.As per coding guidelines, tests should validate behavior (including error/rejection paths), not just setup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/compose-message/hooks/useComposeSend.test.ts` around lines 43 - 65, The renderSend helper function sets up mocked callbacks for inherited resolution (markResolvingInherited, markInheritedResolved, markInheritedFailed) but there are no test cases that actually validate these callbacks are invoked or that test the inherited attachment resolution behavior. Add new test cases that verify: 1) when inherited attachments are pending, the success path resolves them and includes them in the email payload while calling the appropriate callbacks, and 2) when inherited attachment resolution fails, the error is handled correctly and sendEmail is aborted while the failure callback is invoked. These tests should use the renderSend helper and assert on the mock callback invocations and behavior outcomes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/compose-message/helpers/format-email.ts`:
- Line 39: The formatAddress call on line 39 directly accesses mail.from[0]
without verifying that the from array exists and contains at least one element.
Add a guard condition to check if mail.from has elements before attempting to
access mail.from[0]. If the from array is empty, either skip adding this field
to the formatted output or provide a fallback value such as an empty string or a
default sender label to prevent the compose initialization crash.
- Line 55: The hardcoded `Fwd:` prefix in the subject line assignment is
case-sensitive and not localized, which can cause duplicate prefixes like `fwd:`
and prevents proper translation. Extract the hardcoded `Fwd:` string into a
localized translation key that will be defined across all required locales (en,
es, fr, it), then update the startsWith check to perform a case-insensitive
comparison by converting both the mail.subject and the localized prefix to the
same case before checking, ensuring the final subject uses the properly
localized prefix from the translation system.
- Around line 25-42: The formatAddress function interpolates the untrusted
address.name field directly into the HTML string without escaping, and the
formatEmailToForward function similarly interpolates mail.subject without
escaping. Both of these untrusted values should be HTML-escaped before being
included in the formatted strings to prevent HTML injection attacks. Use an HTML
escaping utility function (such as one from a library like html-escape or
lodash) to escape the address.name value in the formatAddress function return
statement and the mail.subject value in the lines array within
formatEmailToForward function before interpolating them into their respective
strings.
In `@src/components/compose-message/hooks/useAttachments.test.ts`:
- Around line 72-74: The test suite for useAttachments hook is missing coverage
for the newly introduced inherited attachment lifecycle paths. Add test cases
that exercise the addInheritedAttachments function and its state transitions
including resolving, resolved, failed states, and the inherited size-limit
validation path. Ensure tests cover the happy path (successful inheritance),
edge cases (boundary conditions for size limits), error paths (failed
inheritance scenarios), and rejection paths (error handling during resolution)
to meet the coding guidelines requirement of comprehensive test coverage for all
logic paths.
In `@src/components/compose-message/hooks/useAttachments.ts`:
- Around line 93-133: Failed inherited attachments get stuck in error state
because markInheritedFailed sets status to 'error' but the send pipeline only
processes items with status 'pending', leaving no way to retry. Add a new
useCallback hook (similar to markResolvingInherited and markInheritedFailed)
that resets a failed inherited attachment's status back to 'pending' so it can
be reprocessed by the send pipeline, allowing users to retry failed inherited
attachments.
In `@src/components/compose-message/hooks/useComposeMessage.test.ts`:
- Line 23: The describe block titles in useComposeMessage.test.ts are not
following the required behavior-style phrasing pattern. Rename the describe
blocks at line 23 and line 196 to use the plain-language "When ..., then ..."
format instead of the current imperative style. For example, "Hydrating from an
existing draft" should be changed to "When hydrating from an existing draft,
then ..." to align with the coding guidelines for test descriptions.
In `@src/components/compose-message/hooks/useComposeMessage.ts`:
- Around line 55-62: Wrap the setInitialValues function with useCallback to
ensure consistency with other callback handlers in this hook and prevent
unnecessary reruns in downstream consumers. Import useCallback from React if not
already imported, then wrap the setInitialValues function definition with
useCallback and provide an empty dependency array since the function only calls
stable state setters (setSubjectValue, setToRecipients, setCcRecipients,
setBccRecipients, setShowCc, setShowBcc).
In `@src/components/compose-message/hooks/useComposeSend.ts`:
- Line 125: Remove the console.error call that logs 'Failed to re-encrypt
inherited attachment' in the useComposeSend hook and replace it with a
centralized error logging utility that adheres to the project's logging
standards. Identify the appropriate error handling or logging service used
elsewhere in the codebase for similar error scenarios and route this error
through that centralized mechanism instead of direct console output.
- Around line 173-174: The send() function calls handleInheritAttachments() to
resolve inherited attachments but does not check whether it succeeded or failed
before proceeding to call sendEmail. If handleInheritAttachments() returns early
due to a failure, the send operation should also be aborted to prevent silently
sending a message without the intended forwarded attachments. Add a validation
check after the await attachmentsToSend assignment that verifies the result is
valid, and if not, return early from the send() function to prevent sendEmail
from being called.
In `@src/components/compose-message/hooks/useInitialComposeState.test.ts`:
- Line 7: The test file contains duplicate mock declarations for `@/i18n` and
duplicate test blocks. Remove the duplicate vi.mock statement for `@/i18n`
(keeping only one instance of the mock that provides the useTranslationContext
hook returning the translate function). Additionally, remove the duplicate set
of three forward-mode tests that appear later in the file (around lines
111-145), keeping only the original instances to eliminate noise and maintenance
overhead.
In `@src/components/compose-message/hooks/useInitialComposeState.ts`:
- Around line 3-6: The relative imports for internal modules should be replaced
with the `@/` alias to follow the repository's coding standards. In the
useInitialComposeState.ts file, replace the relative imports for
formatEmailToForward and formatEmailToReply from '../helpers/format-email' with
`@/components/compose-message/helpers/format-email`, and replace the relative
import for the Recipient type from '../types' with
`@/components/compose-message/types`. The relative import for useAttachments in
the same directory can remain unchanged.
- Around line 62-79: The replyAll case is incorrectly being routed through the
same code path as forward by falling through to the formatEmailToForward
function, which clears recipients and applies forward semantics. Split the
replyAll and forward cases into separate handling blocks: for replyAll, extract
recipients (to, cc, bcc) directly from compose.sourceMail and preserve them in
the returned data object, while keeping the forward case handling as it
currently is with formatEmailToForward and the subject prefix logic.
---
Outside diff comments:
In `@src/components/compose-message/hooks/useComposeSend.test.ts`:
- Around line 43-65: The renderSend helper function sets up mocked callbacks for
inherited resolution (markResolvingInherited, markInheritedResolved,
markInheritedFailed) but there are no test cases that actually validate these
callbacks are invoked or that test the inherited attachment resolution behavior.
Add new test cases that verify: 1) when inherited attachments are pending, the
success path resolves them and includes them in the email payload while calling
the appropriate callbacks, and 2) when inherited attachment resolution fails,
the error is handled correctly and sendEmail is aborted while the failure
callback is invoked. These tests should use the renderSend helper and assert on
the mock callback invocations and behavior outcomes.
In `@src/components/compose-message/index.tsx`:
- Around line 60-121: Extract the forward orchestration logic currently in
ComposeMessageDialog into a custom hook (e.g., useComposeMessageOrchestration or
useForwardMessageOrchestration). Move the three useEffect blocks, the
hydratedForwardAttachmentsRef tracking, and the related state management logic
(setInitialValues, the forward mode editor content setup, and the inherited
attachments hydration via addInheritedAttachments) into this new hook. Have the
hook return the necessary values and callbacks, then update the component to use
this hook instead of managing these concerns directly, keeping the component
focused on rendering only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9815206-306b-43ea-b6d2-63a0bec4654c
📒 Files selected for processing (17)
src/components/compose-message/helpers/format-email.test.tssrc/components/compose-message/helpers/format-email.tssrc/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.tssrc/components/compose-message/hooks/useComposeMessage.test.tssrc/components/compose-message/hooks/useComposeMessage.tssrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/hooks/useInitialComposeState.test.tssrc/components/compose-message/hooks/useInitialComposeState.tssrc/components/compose-message/index.tsxsrc/hooks/mail/usePreviewMailActions.test.tssrc/hooks/mail/usePreviewMailActions.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.json
Comment on lines
+173
to
174
| const attachmentsToSend = await handleInheritAttachments(); | ||
|
|
There was a problem hiding this comment.
Abort sending when inherited attachment resolution fails.
handleInheritAttachments() can return early on failure, but send() continues and still calls sendEmail. That can silently send a message without intended forwarded attachments.
Suggested fix
const attachmentsToSend = await handleInheritAttachments();
+if (!attachmentsToSend) return;📝 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.
Suggested change
| const attachmentsToSend = await handleInheritAttachments(); | |
| const attachmentsToSend = await handleInheritAttachments(); | |
| if (!attachmentsToSend) return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/compose-message/hooks/useComposeSend.ts` around lines 173 -
174, The send() function calls handleInheritAttachments() to resolve inherited
attachments but does not check whether it succeeded or failed before proceeding
to call sendEmail. If handleInheritAttachments() returns early due to a failure,
the send operation should also be aborted to prevent silently sending a message
without the intended forwarded attachments. Add a validation check after the
await attachmentsToSend assignment that verifies the result is valid, and if
not, return early from the send() function to prevent sendEmail from being
called.
|
jzunigax2
approved these changes
Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementing the logic to forward a message. We format the message so the message appears after an indication of a forwarded message (----Forwarded Message---- ....).