[_]: feature/attachments#57
Merged
Merged
Conversation
Deploying mail-web with
|
| Latest commit: |
a003da7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7e6f9cf3.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-attachments.mail-web-ea0.pages.dev |
xabg2
changed the base branch from
feature/upload-attachment-manager
to
feature/custom-hook-to-upload-attachments
jzunigax2
reviewed
Jun 4, 2026
| [toRecipients, ccRecipients, bccRecipients], | ||
| ); | ||
|
|
||
| const encryptionState = useMemo<'none' | 'encrypted' | 'cleartext'>(() => { |
Contributor
There was a problem hiding this comment.
I had added unknown state as well in useComposeSend.ts as that was flagged by coderabbit and actually made sense as it would solve some edge cases. for example when activeDomains is still loading, encryptionState now returns 'none' when !activeDomains so hitting send before that actually skips encryption
Is useComposeSend.ts now dead code?
these are my main concerns, all else relating to uploads looks good
Contributor
Author
There was a problem hiding this comment.
Let's use the custom hook, so we can remove the logic from the component. It should be encapsulated to a custom hook. I will manage the send logic to the custom hook.
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds email attachment support end-to-end: users upload files via a compose dialog file picker, the send pipeline includes completed attachments in both cleartext and encrypted email payloads, and attachments are displayed in mail previews with download functionality. SDK is updated to ^1.17.4 with HTTP retry enabled. ChangesEmail Attachment Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/compose-message/hooks/useComposeSend.test.ts (1)
35-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd tests for attachment filtering and payload inclusion in
send.The helper now injects
attachments, but the suite does not verify the new behavior (done + blobIdincluded, other states excluded) in cleartext/encrypted payloads. This leaves the new send-path logic unprotected.As per coding guidelines: “Test every
.tsfile that contains logic” and “covering happy path, edge cases, error/rejection paths, and boundary values.”🤖 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 35 - 50, Update the renderSend test helper and add unit tests for useComposeSend.send to verify attachment filtering and payload composition: write tests that call renderSend (which returns result and onSent) and invoke result.current.send for both cleartext and encrypted flows, mocking attachments with various states and asserting only attachments with state === "done" contribute a blobId to the final payload (and others are excluded), and assert the constructed payload includes the expected blobId fields and that onSent is called with that payload; reference the renderSend helper, useComposeSend hook, and the send method on result.current to locate where to add these cases.
🤖 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 `@package.json`:
- Line 27: The package.json dependency entry for "`@internxt/sdk`" uses an
invalid/unpublished version spec "^1.17.4"; update the "`@internxt/sdk`"
dependency to a published version (for example "^1.15.10") or to the actual
latest published tag, then run npm install (or yarn) to refresh node_modules and
the lockfile; verify the change by checking the dependency entry "`@internxt/sdk`"
in package.json and confirming the installed version via npm ls or npm info.
In `@src/components/compose-message/hooks/useComposeMessage.ts`:
- Around line 48-55: Add a unit test in
src/components/compose-message/hooks/useComposeMessage.test.ts that uses
renderHook (and act) to call useComposeMessage, sets non-empty values for
subjectValue, toRecipients, ccRecipients, bccRecipients and toggles
showCc/showBcc via the hook’s exposed setters, then calls result.current.clear()
and asserts that subjectValue === '', toRecipients/ccRecipients/bccRecipients
are empty arrays, and showCc/showBcc are false; reference the hook name
useComposeMessage and the clear() function in the test so it explicitly
exercises and verifies the reset behavior.
In `@src/components/compose-message/index.tsx`:
- Line 62: Move the file-picking logic out of the ComposeMessage component into
the useAttachments hook: take the existing fileInputRef and onFilesPicked
handler out of src/components/compose-message/index.tsx and implement them
inside useAttachments (use the existing addFiles call to add selected files),
expose a triggerFilePicker function and a fileInputProps object (containing ref,
type:'file', multiple:true, hidden:true, onChange handler that calls addFiles
and resets e.target.value). In the component, remove fileInputRef/onFilesPicked
and consume const { triggerFilePicker, fileInputProps } = useAttachments(),
render <input {...fileInputProps} /> and wire the file picker button to
onClick={triggerFilePicker} (preserve disabled logic like isSending). Update any
other occurrences of fileInputRef/onFilesPicked in this file accordingly.
In `@src/features/mail/components/mail-preview/preview/index.tsx`:
- Around line 42-68: The Preview component contains download orchestration
(onDownload) and side effects (calling MailService.instance.downloadAttachment,
creating blobs/URLs, and notificationsService.show) which must be extracted;
create a new hook (e.g., useAttachmentDownloader) that accepts mailId and
translate and exposes a downloadAttachment(attachment: EmailAttachment) async
handler that performs the service call, blob/url creation and revoke, and error
notification (ToastType.Error) so Preview only calls the hook-provided handler;
update Preview to call the hook’s downloadAttachment and remove all download
logic from the component.
- Around line 83-88: Replace the mouse-only DownloadSimpleIcon click handler
with a real, keyboard-accessible <button type="button"> that calls
onDownload(attachment); wrap the DownloadSimpleIcon inside that button, move the
onClick to the button, preserve styling via className (e.g.,
className="cursor-pointer" or better "btn-icon"), and add an accessible label
using translate('mail.downloadAttachment') (or a similar i18n key) passed to
aria-label; then add that i18n key and translated strings to all locales (en,
es, fr, it).
In `@src/test-utils/fixtures.ts`:
- Around line 169-171: The fixture randomly sets hasAttachment and attachments
independently causing inconsistent mail states; compute attachments first (using
the existing attachments expression: faker.datatype.boolean() ?
[getMockedAttachment()] : []) and then set hasAttachment to reflect the actual
list (e.g., hasAttachment = attachments.length > 0) so tests see a consistent
state; update the mail fixture so references to attachments and hasAttachment
use those names (attachments, hasAttachment) in that order.
---
Outside diff comments:
In `@src/components/compose-message/hooks/useComposeSend.test.ts`:
- Around line 35-50: Update the renderSend test helper and add unit tests for
useComposeSend.send to verify attachment filtering and payload composition:
write tests that call renderSend (which returns result and onSent) and invoke
result.current.send for both cleartext and encrypted flows, mocking attachments
with various states and asserting only attachments with state === "done"
contribute a blobId to the final payload (and others are excluded), and assert
the constructed payload includes the expected blobId fields and that onSent is
called with that payload; reference the renderSend helper, useComposeSend hook,
and the send method on result.current to locate where to add these cases.
🪄 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: 78593500-2e2b-4bda-8b7a-18135fe1f83a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonsrc/components/compose-message/components/AttachmentList.tsxsrc/components/compose-message/hooks/useComposeMessage.tssrc/components/compose-message/hooks/useComposeSend.test.tssrc/components/compose-message/hooks/useComposeSend.tssrc/components/compose-message/index.tsxsrc/features/mail/MailView.tsxsrc/features/mail/components/mail-preview/index.tsxsrc/features/mail/components/mail-preview/preview/index.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/index.tssrc/test-utils/fixtures.ts
jzunigax2
approved these changes
Jun 4, 2026
Base automatically changed from
feature/custom-hook-to-upload-attachments
to
master
June 4, 2026 13:55
|
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.
Allowing the user to send attachments when composing an email and download attachments from an email. The limit is set to <25MB.