[PB-1967]: feature/custom hook to upload attachments#59
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR implements a complete attachment upload system for mail compose: type contracts and a 25 MiB limit constant, a concurrency-managed UploadManager with retry/cancel semantics, a useAttachments React hook enforcing size validation, tests, and localized error messages (en/es/fr/it). ChangesAttachment Upload Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (2)
41-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winJustify
asyncbefore adding two new packages for one queue.The new upload manager only consumes
queue/QueueObjectfromasyncinsrc/services/upload-manager/index.ts. That concurrency wrapper is small enough to keep local, so adding bothasyncand@types/asyncdoes not meet the repo’s dependency policy unless you document a concrete need that a small in-repo implementation cannot cover. As per coding guidelines, "Dependency changes: don’t add packages unless justified by real problem/complexity vs implementing in ≤~100 LOC, and ensure TypeScript support/maintenance/licensing are acceptable."🤖 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 `@package.json` around lines 41 - 69, Remove the new external dependency addition ("async" and "`@types/async`") unless you can justify it; instead implement a small in-repo concurrency wrapper used by src/services/upload-manager/index.ts (exporting the queue/QueueObject API the code expects) — create a local utility (e.g., upload-manager/queue.ts) that provides a createQueue(concurrency) or queue(tasks, concurrency) with push/idle/drain semantics and appropriate TypeScript types matching QueueObject, update imports in index.ts to use the local implementation, and add a short unit test; if you choose to keep the external package, add a one-paragraph justification in the PR explaining why an in-repo ~100 LOC implementation is insufficient and confirm TypeScript typings and license compatibility instead of adding the packages silently.
41-69:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBump
@internxt/sdkor rewrite the attachment integration to match1.17.2.@internxt/sdk@1.17.2’s TypeScript declarations don’t defineMailApi.uploadAttachment/downloadAttachmentnorUploadAttachmentResponse/DownloadAttachmentResponse, but this code imports/types/calls them (e.g.,src/services/sdk/mail/index.ts,src/services/upload-manager/index.ts), sonpm run buildcan’t pass without upgrading the SDK or adapting to the 1.17.2 API.🤖 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 `@package.json` around lines 41 - 69, The build breaks because the code references MailApi.uploadAttachment, MailApi.downloadAttachment and types UploadAttachmentResponse/DownloadAttachmentResponse (used in src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts) but `@internxt/sdk`@1.17.2’s typings don’t expose them; fix by either updating package.json to a compatible `@internxt/sdk` version that includes those methods/types (bump to the SDK release that added uploadAttachment/downloadAttachment) or refactor the two modules to use the current 1.17.2 SDK API and exported types (replace calls/types with the SDK’s actual attachment upload/download functions and response types), ensuring all imports and type references in src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts are updated accordingly.
🤖 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/hooks/useAttachments.test.ts`:
- Around line 35-50: Test mocks are only restored in afterEach, allowing
spies/mocks from a test to leak into the next test; move vi.restoreAllMocks()
into the beforeEach and reapply the shared run.mockImplementation setup there so
each test starts with a clean slate. Update the beforeEach block to call
vi.restoreAllMocks() at its start, then reset the specific mocks (run, retry,
remove, clear, show), set lastCallbacks = undefined, and re-establish
run.mockImplementation(...) as currently written; remove or keep
afterEach.restore depending on preference but ensure beforeEach restores mocks
before applying run.mockImplementation.
- Line 3: The test is using a relative internal import for the useAttachments
hook instead of the repository alias. Update the import in
useAttachments.test.ts to use the
`@/components/compose-message/hooks/useAttachments` path so it follows the
internal-module import rule and stays consistent with the rest of the repo.
In `@src/components/compose-message/hooks/useAttachments.ts`:
- Line 60: The MIME type fallback is wrong because browsers set File.type to an
empty string; update the assignment that currently uses "type: file.type ??
'application/octet-stream'" in useAttachments.ts to use a truthy fallback so
empty strings fall back (e.g., use file.type || 'application/octet-stream'),
ensuring the file.type check treats '' as unknown; locate the assignment where
the File object (variable name "file") is mapped and replace the nullish
coalescing with a truthy OR for the MIME type.
- Around line 55-63: The issue is that UploadManager.instance.run(...) can
invoke onSuccess/onError before setAttachments(...pending) commits, so callbacks
map an empty state and leave tasks stuck; fix by creating the AttachmentTask
entries and registering them in state before starting the upload: generate
handles/IDs (or map the incoming file list to pending AttachmentTask objects
with stable ids), call setAttachments(prev => [...prev, ...pending]) first, then
invoke UploadManager.instance.run(...) passing callbacks that reference those
same ids (or modify run to accept the precomputed ids/handles). Update code
around UploadManager.instance.run, handles, pending, setAttachments, and the
onSuccess/onError callback logic to rely on pre-registered AttachmentTask ids.
- Line 6: Remove the invalid SDK import of AttachmentRef from useAttachments.ts
and replace it with a local inline type that matches what the hook actually
uses: an interface (e.g., LocalAttachment or AttachmentShape) with properties
name: string, size: number, type: string, and an optional blobId?: string; then
update the useAttachments hook code (references to AttachmentRef) to use this
local type so the file compiles without referencing `@internxt/sdk` exports.
In `@src/services/upload-manager/index.ts`:
- Around line 95-110: The uploadWithRetries function currently retries transient
errors synchronously; modify uploadWithRetries to wait with a bounded
exponential backoff plus jitter before each retry attempt (only when
isTransientError(error) and task.cancelled is false) instead of looping
immediately; compute backoff using attempt index (capped to a MAX_BACKOFF_MS)
and add random jitter, await a sleep/timeout before creating the next
MailService.instance.uploadAttachment promise, and preserve existing behavior
for non-transient errors and cancelation while keeping task.canceler cleared in
the finally block.
In `@src/services/upload-manager/upload-manager.test.ts`:
- Line 2: The test imports the UploadManager via a relative path; change the
import to use the repository path alias instead. Update the import statement
that currently references './index' to import UploadManager from
'`@/services/upload-manager`' so the test uses the project-wide alias and matches
other internal module imports (symbol: UploadManager).
- Around line 30-37: Tests leave mutable state in the UploadManager singleton
(UploadManager.instance) between runs so resetting mocks isn't enough; call
UploadManager.instance.clear() in the test lifecycle (e.g., add
UploadManager.instance.clear() to beforeEach or afterEach alongside
vi.restoreAllMocks() and vi.useRealTimers()) to purge queued/failed tasks and
ensure test isolation, referencing UploadManager.instance.clear() to locate the
method to invoke.
In `@src/types/mail/upload-manager/index.ts`:
- Around line 13-20: The exported type UploadAttachmentTask exposes internal
mutable state (cancelled, failed) that should be encapsulated by UploadManager;
either move the UploadAttachmentTask definition into
src/services/upload-manager/index.ts and make it non-exported so only
UploadManager manipulates it, or mark it `@internal` in its JSDoc and document
that external mutation is unsupported, or change the shape so fields are
readonly and have UploadManager maintain mutable state separately (refer to
UploadManager, UploadAttachmentCallbacks, and RequestCanceler to locate usages)
— pick one approach and update all imports/usages accordingly to prevent
external code from mutating the manager's internal state.
---
Outside diff comments:
In `@package.json`:
- Around line 41-69: Remove the new external dependency addition ("async" and
"`@types/async`") unless you can justify it; instead implement a small in-repo
concurrency wrapper used by src/services/upload-manager/index.ts (exporting the
queue/QueueObject API the code expects) — create a local utility (e.g.,
upload-manager/queue.ts) that provides a createQueue(concurrency) or
queue(tasks, concurrency) with push/idle/drain semantics and appropriate
TypeScript types matching QueueObject, update imports in index.ts to use the
local implementation, and add a short unit test; if you choose to keep the
external package, add a one-paragraph justification in the PR explaining why an
in-repo ~100 LOC implementation is insufficient and confirm TypeScript typings
and license compatibility instead of adding the packages silently.
- Around line 41-69: The build breaks because the code references
MailApi.uploadAttachment, MailApi.downloadAttachment and types
UploadAttachmentResponse/DownloadAttachmentResponse (used in
src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts) but
`@internxt/sdk`@1.17.2’s typings don’t expose them; fix by either updating
package.json to a compatible `@internxt/sdk` version that includes those
methods/types (bump to the SDK release that added
uploadAttachment/downloadAttachment) or refactor the two modules to use the
current 1.17.2 SDK API and exported types (replace calls/types with the SDK’s
actual attachment upload/download functions and response types), ensuring all
imports and type references in src/services/sdk/mail/index.ts and
src/services/upload-manager/index.ts are updated accordingly.
🪄 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: ce24a1b5-0c77-4fe5-9404-da20d6dacbfb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonsrc/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.tssrc/constants.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/mail/index.tssrc/services/upload-manager/index.tssrc/services/upload-manager/upload-manager.test.tssrc/types/mail/upload-manager/index.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (2)
41-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winJustify
asyncbefore adding two new packages for one queue.The new upload manager only consumes
queue/QueueObjectfromasyncinsrc/services/upload-manager/index.ts. That concurrency wrapper is small enough to keep local, so adding bothasyncand@types/asyncdoes not meet the repo’s dependency policy unless you document a concrete need that a small in-repo implementation cannot cover. As per coding guidelines, "Dependency changes: don’t add packages unless justified by real problem/complexity vs implementing in ≤~100 LOC, and ensure TypeScript support/maintenance/licensing are acceptable."🤖 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 `@package.json` around lines 41 - 69, Remove the new external dependency addition ("async" and "`@types/async`") unless you can justify it; instead implement a small in-repo concurrency wrapper used by src/services/upload-manager/index.ts (exporting the queue/QueueObject API the code expects) — create a local utility (e.g., upload-manager/queue.ts) that provides a createQueue(concurrency) or queue(tasks, concurrency) with push/idle/drain semantics and appropriate TypeScript types matching QueueObject, update imports in index.ts to use the local implementation, and add a short unit test; if you choose to keep the external package, add a one-paragraph justification in the PR explaining why an in-repo ~100 LOC implementation is insufficient and confirm TypeScript typings and license compatibility instead of adding the packages silently.
41-69:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftBump
@internxt/sdkor rewrite the attachment integration to match1.17.2.@internxt/sdk@1.17.2’s TypeScript declarations don’t defineMailApi.uploadAttachment/downloadAttachmentnorUploadAttachmentResponse/DownloadAttachmentResponse, but this code imports/types/calls them (e.g.,src/services/sdk/mail/index.ts,src/services/upload-manager/index.ts), sonpm run buildcan’t pass without upgrading the SDK or adapting to the 1.17.2 API.🤖 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 `@package.json` around lines 41 - 69, The build breaks because the code references MailApi.uploadAttachment, MailApi.downloadAttachment and types UploadAttachmentResponse/DownloadAttachmentResponse (used in src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts) but `@internxt/sdk`@1.17.2’s typings don’t expose them; fix by either updating package.json to a compatible `@internxt/sdk` version that includes those methods/types (bump to the SDK release that added uploadAttachment/downloadAttachment) or refactor the two modules to use the current 1.17.2 SDK API and exported types (replace calls/types with the SDK’s actual attachment upload/download functions and response types), ensuring all imports and type references in src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts are updated accordingly.
🤖 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/hooks/useAttachments.test.ts`:
- Around line 35-50: Test mocks are only restored in afterEach, allowing
spies/mocks from a test to leak into the next test; move vi.restoreAllMocks()
into the beforeEach and reapply the shared run.mockImplementation setup there so
each test starts with a clean slate. Update the beforeEach block to call
vi.restoreAllMocks() at its start, then reset the specific mocks (run, retry,
remove, clear, show), set lastCallbacks = undefined, and re-establish
run.mockImplementation(...) as currently written; remove or keep
afterEach.restore depending on preference but ensure beforeEach restores mocks
before applying run.mockImplementation.
- Line 3: The test is using a relative internal import for the useAttachments
hook instead of the repository alias. Update the import in
useAttachments.test.ts to use the
`@/components/compose-message/hooks/useAttachments` path so it follows the
internal-module import rule and stays consistent with the rest of the repo.
In `@src/components/compose-message/hooks/useAttachments.ts`:
- Line 60: The MIME type fallback is wrong because browsers set File.type to an
empty string; update the assignment that currently uses "type: file.type ??
'application/octet-stream'" in useAttachments.ts to use a truthy fallback so
empty strings fall back (e.g., use file.type || 'application/octet-stream'),
ensuring the file.type check treats '' as unknown; locate the assignment where
the File object (variable name "file") is mapped and replace the nullish
coalescing with a truthy OR for the MIME type.
- Around line 55-63: The issue is that UploadManager.instance.run(...) can
invoke onSuccess/onError before setAttachments(...pending) commits, so callbacks
map an empty state and leave tasks stuck; fix by creating the AttachmentTask
entries and registering them in state before starting the upload: generate
handles/IDs (or map the incoming file list to pending AttachmentTask objects
with stable ids), call setAttachments(prev => [...prev, ...pending]) first, then
invoke UploadManager.instance.run(...) passing callbacks that reference those
same ids (or modify run to accept the precomputed ids/handles). Update code
around UploadManager.instance.run, handles, pending, setAttachments, and the
onSuccess/onError callback logic to rely on pre-registered AttachmentTask ids.
- Line 6: Remove the invalid SDK import of AttachmentRef from useAttachments.ts
and replace it with a local inline type that matches what the hook actually
uses: an interface (e.g., LocalAttachment or AttachmentShape) with properties
name: string, size: number, type: string, and an optional blobId?: string; then
update the useAttachments hook code (references to AttachmentRef) to use this
local type so the file compiles without referencing `@internxt/sdk` exports.
In `@src/services/upload-manager/index.ts`:
- Around line 95-110: The uploadWithRetries function currently retries transient
errors synchronously; modify uploadWithRetries to wait with a bounded
exponential backoff plus jitter before each retry attempt (only when
isTransientError(error) and task.cancelled is false) instead of looping
immediately; compute backoff using attempt index (capped to a MAX_BACKOFF_MS)
and add random jitter, await a sleep/timeout before creating the next
MailService.instance.uploadAttachment promise, and preserve existing behavior
for non-transient errors and cancelation while keeping task.canceler cleared in
the finally block.
In `@src/services/upload-manager/upload-manager.test.ts`:
- Line 2: The test imports the UploadManager via a relative path; change the
import to use the repository path alias instead. Update the import statement
that currently references './index' to import UploadManager from
'`@/services/upload-manager`' so the test uses the project-wide alias and matches
other internal module imports (symbol: UploadManager).
- Around line 30-37: Tests leave mutable state in the UploadManager singleton
(UploadManager.instance) between runs so resetting mocks isn't enough; call
UploadManager.instance.clear() in the test lifecycle (e.g., add
UploadManager.instance.clear() to beforeEach or afterEach alongside
vi.restoreAllMocks() and vi.useRealTimers()) to purge queued/failed tasks and
ensure test isolation, referencing UploadManager.instance.clear() to locate the
method to invoke.
In `@src/types/mail/upload-manager/index.ts`:
- Around line 13-20: The exported type UploadAttachmentTask exposes internal
mutable state (cancelled, failed) that should be encapsulated by UploadManager;
either move the UploadAttachmentTask definition into
src/services/upload-manager/index.ts and make it non-exported so only
UploadManager manipulates it, or mark it `@internal` in its JSDoc and document
that external mutation is unsupported, or change the shape so fields are
readonly and have UploadManager maintain mutable state separately (refer to
UploadManager, UploadAttachmentCallbacks, and RequestCanceler to locate usages)
— pick one approach and update all imports/usages accordingly to prevent
external code from mutating the manager's internal state.
---
Outside diff comments:
In `@package.json`:
- Around line 41-69: Remove the new external dependency addition ("async" and
"`@types/async`") unless you can justify it; instead implement a small in-repo
concurrency wrapper used by src/services/upload-manager/index.ts (exporting the
queue/QueueObject API the code expects) — create a local utility (e.g.,
upload-manager/queue.ts) that provides a createQueue(concurrency) or
queue(tasks, concurrency) with push/idle/drain semantics and appropriate
TypeScript types matching QueueObject, update imports in index.ts to use the
local implementation, and add a short unit test; if you choose to keep the
external package, add a one-paragraph justification in the PR explaining why an
in-repo ~100 LOC implementation is insufficient and confirm TypeScript typings
and license compatibility instead of adding the packages silently.
- Around line 41-69: The build breaks because the code references
MailApi.uploadAttachment, MailApi.downloadAttachment and types
UploadAttachmentResponse/DownloadAttachmentResponse (used in
src/services/sdk/mail/index.ts and src/services/upload-manager/index.ts) but
`@internxt/sdk`@1.17.2’s typings don’t expose them; fix by either updating
package.json to a compatible `@internxt/sdk` version that includes those
methods/types (bump to the SDK release that added
uploadAttachment/downloadAttachment) or refactor the two modules to use the
current 1.17.2 SDK API and exported types (replace calls/types with the SDK’s
actual attachment upload/download functions and response types), ensuring all
imports and type references in src/services/sdk/mail/index.ts and
src/services/upload-manager/index.ts are updated accordingly.
🪄 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: ce24a1b5-0c77-4fe5-9404-da20d6dacbfb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonsrc/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.tssrc/constants.tssrc/i18n/locales/en.jsonsrc/i18n/locales/es.jsonsrc/i18n/locales/fr.jsonsrc/i18n/locales/it.jsonsrc/services/sdk/mail/index.tssrc/services/upload-manager/index.tssrc/services/upload-manager/upload-manager.test.tssrc/types/mail/upload-manager/index.ts
🛑 Comments failed to post (9)
src/components/compose-message/hooks/useAttachments.test.ts (2)
3-3: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use the repo alias for this internal import.
This test should import the hook via
@/components/compose-message/hooks/useAttachmentsto stay consistent with the repository’s internal-module import rule. As per coding guidelines, “Use the path alias@/*→src/*when importing internal modules.”🤖 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/useAttachments.test.ts` at line 3, The test is using a relative internal import for the useAttachments hook instead of the repository alias. Update the import in useAttachments.test.ts to use the `@/components/compose-message/hooks/useAttachments` path so it follows the internal-module import rule and stays consistent with the rest of the repo.
35-50: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Restore mocks in
beforeEach, not only inafterEach.Right now only the four known mocks are reset before each test. Any future spy/mock added in a test can still bleed into the next Arrange phase until
afterEachruns. Movevi.restoreAllMocks()intobeforeEachand re-apply the sharedrun.mockImplementation(...)setup there. Based on learnings, “Reset mocks in beforeEach with vi.restoreAllMocks().”🤖 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/useAttachments.test.ts` around lines 35 - 50, Test mocks are only restored in afterEach, allowing spies/mocks from a test to leak into the next test; move vi.restoreAllMocks() into the beforeEach and reapply the shared run.mockImplementation setup there so each test starts with a clean slate. Update the beforeEach block to call vi.restoreAllMocks() at its start, then reset the specific mocks (run, retry, remove, clear, show), set lastCallbacks = undefined, and re-establish run.mockImplementation(...) as currently written; remove or keep afterEach.restore depending on preference but ensure beforeEach restores mocks before applying run.mockImplementation.src/components/compose-message/hooks/useAttachments.ts (3)
6-6:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick win🧩 Analysis chain
🌐 Web query:
In@internxt/sdkversion 1.17.2, what types are exported from@internxt/sdk/dist/mail/types, and what is the correct replacement forAttachmentRef?💡 Result:
In
@internxt/sdkversion 1.17.2, the@internxt/sdk/dist/mail/typesmodule is used for mail-related type definitions. While a specific public replacement for the deprecatedAttachmentRefis not explicitly documented in the official SDK repository's public release notes or current export files, the Internxt SDK architecture extensively utilizes standardized interfaces for handling file attachments [1][2]. Users encountering issues withAttachmentRefshould examine the updated attachment-handling interfaces within the SDK's shared type definitions or the specific mail service modules. The SDK has undergone significant refactoring to align its types with backend Swagger schemas [3], which typically involves renaming or replacing older, custom reference types with standardized objects containing fields likeid,filename, andsize[4]. It is recommended to replaceAttachmentRefwith the relevantAttachmentorAttachmentDatainterface currently exported by the SDK, ensuring the new type aligns with the current backend schema expectations [4][5]. For specific implementation guidance, reviewing the current exports in@internxt/sdk/dist/mail/typesdirectly via IDE autocompletion or by inspecting the localnode_modules/@internxt/sdk/dist/mail/types.d.tsfile is the most authoritative way to identify the current type name.Citations:
- 1: https://github.com/internxt/sdk/tree/master
- 2: https://github.com/internxt/sdk/
- 3: internxt/sdk#329
- 4: resend/resend-node@af22de3
- 5: internxt/sdk#272
🏁 Script executed:
#!/bin/bash set -euo pipefail # 1) Find any usage of AttachmentRef in the repo rg -n "AttachmentRef" . || true # 2) Try to locate the SDK type definition in the repo (if node_modules exist in this sandbox) # We intentionally keep output small and only print the relevant d.ts file(s). if [ -d "node_modules/@internxt/sdk" ]; then fd -a "dist/mail/types.*" node_modules/@internxt/sdk -t f || true # Print matching d.ts contents if found fd -a "dist/mail/types.*\.d\.ts$" node_modules/@internxt/sdk -t f -x sh -c 'echo "==== $1 ===="; sed -n "1,200p" "$1"' -- {} || true # Also search within the found types.d.ts for exported symbols fd -a "dist/mail/types.*\.d\.ts$" node_modules/@internxt/sdk -t f -x sh -c 'rg -n "export (type|interface|class) " "$1" || true' -- {} || true else echo "node_modules/@internxt/sdk not present in this sandbox; skipping local .d.ts inspection." fiRepository: internxt/mail-web
Length of output: 320
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Check node_modules presence ==" ls -la node_modules/@internxt 2>/dev/null || true ls -la node_modules/@internxt/sdk 2>/dev/null || true echo echo "== Look for SDK dist/mail/types typings ==" if [ -d "node_modules/@internxt/sdk/dist/mail" ]; then fd -a "types*.d.ts" node_modules/@internxt/sdk/dist/mail -t f || true fd -a "types*.ts" node_modules/@internxt/sdk/dist/mail -t f || true else echo "No node_modules/@internxt/sdk/dist/mail in this environment." fi echo echo "== If a types.d.ts exists, print first 200 lines ==" if [ -d "node_modules/@internxt/sdk/dist/mail" ]; then fd -a "types*.d.ts" node_modules/@internxt/sdk/dist/mail -t f | head -n 5 | while read -r f; do echo "---- $f ----" sed -n '1,200p' "$f" done fiRepository: internxt/mail-web
Length of output: 4289
🏁 Script executed:
#!/bin/bash set -euo pipefail TYPES_DTS="node_modules/@internxt/sdk/dist/mail/types.d.ts" echo "== Search for AttachmentRef ==" rg -n "AttachmentRef" "$TYPES_DTS" || true echo echo "== Search for blobId ==" rg -n "blobId" "$TYPES_DTS" || true echo echo "== Search for Attachment-related exports (Attachment/AttachmentDto/encAttachments) ==" rg -n "export type .*Attachment|Attachment.*Dto|encAttachments|attachments" "$TYPES_DTS" || true echo echo "== Show matching lines around any blobId occurrences (if present) ==" if rg -n "blobId" "$TYPES_DTS" >/dev/null 2>&1; then rg -n -C 3 "blobId" "$TYPES_DTS" fiRepository: internxt/mail-web
Length of output: 308
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== useAttachments.ts (show relevant sections with line numbers) ==" FILE="src/components/compose-message/hooks/useAttachments.ts" nl -ba "$FILE" | sed -n '1,140p' echo echo "== Search SDK typings for attachment/attachment dto/blobId symbols ==" SDK_MAIL_DIR="node_modules/@internxt/sdk/dist/mail" rg -n "AttachmentRef|AttachmentDto|BlobId|blobId|attachments" "$SDK_MAIL_DIR" --glob '*.d.ts' || true echo echo "== If schema typings exist, search there too (broader) ==" rg -n "Attachment|blobId|BlobId" "$SDK_MAIL_DIR" --glob '*.ts' --glob '*.d.ts' || trueRepository: internxt/mail-web
Length of output: 189
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== useAttachments.ts (show relevant sections with line numbers) ==" FILE="src/components/compose-message/hooks/useAttachments.ts" cat -n "$FILE" | sed -n '1,200p' echo echo "== Search SDK typings for attachment/attachment dto/blobId symbols ==" SDK_MAIL_DIR="node_modules/@internxt/sdk/dist/mail" rg -n "AttachmentRef|AttachmentDto|BlobId|blobId|encAttachments|Attachment" "$SDK_MAIL_DIR" --glob '*.d.ts' --glob '*.ts' || trueRepository: internxt/mail-web
Length of output: 4586
🏁 Script executed:
#!/bin/bash set -euo pipefail SDK_MAIL_DIR="node_modules/@internxt/sdk/dist/mail" echo "== Find likely attachment DTO/type names in schema.d.ts ==" rg -n "Attachment|attachment" "$SDK_MAIL_DIR/schema.d.ts" || true echo echo "== List exported component schema aliases that include 'Attachment' ==" rg -n "export type .*Attachment" "$SDK_MAIL_DIR/schema.d.ts" || trueRepository: internxt/mail-web
Length of output: 487
🏁 Script executed:
#!/bin/bash set -euo pipefail SDK_DIR="node_modules/@internxt/sdk/dist/mail" echo "== Search for AttachmentRef anywhere in sdk dist/mail ==" rg -n "AttachmentRef" "$SDK_DIR" --glob '*.d.ts' --glob '*.ts' --glob '*.js' || true echo echo "== Search for blobId in sdk dist/mail ==" rg -n "blobId" "$SDK_DIR" --glob '*.d.ts' --glob '*.ts' || true echo echo "== Search for 'Attachment' identifier in sdk dist/mail typdefs ==" rg -n "Attachment" "$SDK_DIR" --glob '*.d.ts' --glob '*.ts' || true echo echo "== Search for file fields used by the hook (name/size/type) in sdk mail d.ts ==" rg -n "name: string;|size:|type:" "$SDK_DIR" --glob '*.d.ts' || trueRepository: internxt/mail-web
Length of output: 1265
Fix broken SDK type import in
useAttachments(TS2305 build blocker)
src/components/compose-message/hooks/useAttachments.tsimportsAttachmentReffrom@internxt/sdk/dist/mail/types, butAttachmentRefis not exported by the SDK typings (the module only exports mail DTO/operation types). Remove theAttachmentRefimport and inline a local attachment shape for what this hook actually needs (name,size,type, optionalblobId) so the hook compiles.🧰 Tools
🪛 GitHub Actions: Mail Web CI / 0_ci (24.x).txt
[error] 6-6: TypeScript (tsc -b) failed: TS2305 Module "
@internxt/sdk/dist/mail/types" has no exported member "AttachmentRef".🪛 GitHub Actions: Mail Web CI / ci (24.x)
[error] 6-6: TypeScript build failed (tsc -b). TS2305: Module "
@internxt/sdk/dist/mail/types" has no exported member "AttachmentRef".🪛 GitHub Check: ci (24.x)
[failure] 6-6:
Module '"@internxt/sdk/dist/mail/types"' has no exported member 'AttachmentRef'.🤖 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/useAttachments.ts` at line 6, Remove the invalid SDK import of AttachmentRef from useAttachments.ts and replace it with a local inline type that matches what the hook actually uses: an interface (e.g., LocalAttachment or AttachmentShape) with properties name: string, size: number, type: string, and an optional blobId?: string; then update the useAttachments hook code (references to AttachmentRef) to use this local type so the file compiles without referencing `@internxt/sdk` exports.
55-63:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRegister local tasks before uploads can complete.
UploadManager.instance.run()starts work immediately, and its callbacks can fire before thesetAttachments(...pending)update is committed. In that caseonSuccess/onErrormaps over an empty array, drops the transition, and the later append leaves the task stuck inuploading. This needs an API/order change so IDs exist in state before the manager can call back.🤖 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/useAttachments.ts` around lines 55 - 63, The issue is that UploadManager.instance.run(...) can invoke onSuccess/onError before setAttachments(...pending) commits, so callbacks map an empty state and leave tasks stuck; fix by creating the AttachmentTask entries and registering them in state before starting the upload: generate handles/IDs (or map the incoming file list to pending AttachmentTask objects with stable ids), call setAttachments(prev => [...prev, ...pending]) first, then invoke UploadManager.instance.run(...) passing callbacks that reference those same ids (or modify run to accept the precomputed ids/handles). Update code around UploadManager.instance.run, handles, pending, setAttachments, and the onSuccess/onError callback logic to rely on pre-registered AttachmentTask ids.
60-60:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a truthy fallback for unknown MIME types.
Browsers report unknown
File.typeas'', notundefined, so??preserves the empty string and never falls back toapplication/octet-stream. Usefile.type || 'application/octet-stream'here.🤖 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/useAttachments.ts` at line 60, The MIME type fallback is wrong because browsers set File.type to an empty string; update the assignment that currently uses "type: file.type ?? 'application/octet-stream'" in useAttachments.ts to use a truthy fallback so empty strings fall back (e.g., use file.type || 'application/octet-stream'), ensuring the file.type check treats '' as unknown; locate the assignment where the File object (variable name "file") is mapped and replace the nullish coalescing with a truthy OR for the MIME type.src/services/upload-manager/index.ts (1)
95-110:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd bounded backoff between transient retry attempts.
uploadWithRetries()retries 5xx/network failures in a tight loop. That turns one transient outage into three immediate requests per file, which both contradicts the PR’s stated retry behavior and can amplify backend pressure during incidents. Insert a delay/jitter before each retry instead of retrying synchronously.🤖 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/services/upload-manager/index.ts` around lines 95 - 110, The uploadWithRetries function currently retries transient errors synchronously; modify uploadWithRetries to wait with a bounded exponential backoff plus jitter before each retry attempt (only when isTransientError(error) and task.cancelled is false) instead of looping immediately; compute backoff using attempt index (capped to a MAX_BACKOFF_MS) and add random jitter, await a sleep/timeout before creating the next MailService.instance.uploadAttachment promise, and preserve existing behavior for non-transient errors and cancelation while keeping task.canceler cleared in the finally block.src/services/upload-manager/upload-manager.test.ts (2)
2-2: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use the repo path alias for this internal import.
Import
UploadManagerthrough@/services/upload-managerinstead of./indexto match the repo-wide alias rule. As per coding guidelines, "Use the path alias@/*→src/*when importing internal modules."🤖 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/services/upload-manager/upload-manager.test.ts` at line 2, The test imports the UploadManager via a relative path; change the import to use the repository path alias instead. Update the import statement that currently references './index' to import UploadManager from '`@/services/upload-manager`' so the test uses the project-wide alias and matches other internal module imports (symbol: UploadManager).
30-37:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the singleton manager between tests.
UploadManager.instancekeeps mutable queue/task state across the whole suite, and failed uploads remain stored untilretry/remove/clear. Resetting mocks alone is not enough here, so these cases can become order-dependent. AddUploadManager.instance.clear()in test setup/teardown to keep the suite isolated.🤖 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/services/upload-manager/upload-manager.test.ts` around lines 30 - 37, Tests leave mutable state in the UploadManager singleton (UploadManager.instance) between runs so resetting mocks isn't enough; call UploadManager.instance.clear() in the test lifecycle (e.g., add UploadManager.instance.clear() to beforeEach or afterEach alongside vi.restoreAllMocks() and vi.useRealTimers()) to purge queued/failed tasks and ensure test isolation, referencing UploadManager.instance.clear() to locate the method to invoke.src/types/mail/upload-manager/index.ts (1)
13-20: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider making
UploadAttachmentTaskinternal to the UploadManager.This type contains mutable state fields (
cancelled,failed) that are managed internally by the UploadManager. Exporting it exposes implementation details and could allow external code to mutate these fields, potentially breaking the manager's state guarantees.Consider one of:
- Move this type into
src/services/upload-manager/index.tsas a non-exported type- Add a JSDoc comment marking it as
@internaland documenting that external mutation is unsupported- Refactor to use readonly fields with the manager maintaining mutable state separately
🤖 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/types/mail/upload-manager/index.ts` around lines 13 - 20, The exported type UploadAttachmentTask exposes internal mutable state (cancelled, failed) that should be encapsulated by UploadManager; either move the UploadAttachmentTask definition into src/services/upload-manager/index.ts and make it non-exported so only UploadManager manipulates it, or mark it `@internal` in its JSDoc and document that external mutation is unsupported, or change the shape so fields are readonly and have UploadManager maintain mutable state separately (refer to UploadManager, UploadAttachmentCallbacks, and RequestCanceler to locate usages) — pick one approach and update all imports/usages accordingly to prevent external code from mutating the manager's internal state.
Deploying mail-web with
|
| Latest commit: |
a6c8c21
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5c11294b.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-custom-hook-to-uploa.mail-web-ea0.pages.dev |
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: 1
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/hooks/useAttachments.test.ts (2)
35-50: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReset mocks in
beforeEachusingvi.restoreAllMocks()per test policy.Current setup restores in
afterEach; move restore tobeforeEachto match the required test lifecycle rule.Suggested fix
beforeEach(() => { + vi.restoreAllMocks(); run.mockReset(); retry.mockReset(); remove.mockReset(); clear.mockReset(); show.mockReset(); lastCallbacks = undefined; @@ - afterEach(() => { - vi.restoreAllMocks(); - });As per coding guidelines, “Reset mocks in beforeEach with
vi.restoreAllMocks().”🤖 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/useAttachments.test.ts` around lines 35 - 50, Move the vi.restoreAllMocks() call from the afterEach block into the beforeEach block so mocks are restored at test setup; specifically, in the test file adjust the lifecycle hooks so beforeEach calls vi.restoreAllMocks() before run.mockReset()/retry.mockReset()/remove.mockReset()/clear.mockReset()/show.mockReset() and the afterEach no longer calls vi.restoreAllMocks(); update references in the setup where run.mockImplementation and lastCallbacks are initialized to run after the restore.
3-3: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the project alias for internal imports instead of relative paths.
./useAttachmentsshould use the@/*alias policy forsrcimports.Suggested fix
-import useAttachments from './useAttachments'; +import useAttachments from '`@/components/compose-message/hooks/useAttachments`';As per coding guidelines, “Use the path alias
@/*→src/*when importing internal modules.”🤖 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/useAttachments.test.ts` at line 3, Update the internal import in useAttachments.test.ts to use the project alias instead of a relative path: replace import useAttachments from './useAttachments' with the aliased path import useAttachments from '`@/components/compose-message/hooks/useAttachments`' so it follows the `@/*` → `src/*` convention and resolves correctly across the codebase.
🤖 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/hooks/useAttachments.ts`:
- Around line 23-26: The current totalSize memo only sums attachments with
status === 'done', so the limit check later (referencing totalSize) ignores
in-flight uploads and allows final size to exceed 25 MiB; update the useMemo
that computes totalSize (and any checks that reference it) to include
attachments with status 'uploading' (and any other non-failed/non-removed states
you use) so in-flight bytes are counted (or create a new variable like
totalSizeIncludingUploading and use that in the limit check), then use that
value in the existing limit enforcement logic to prevent accepting batches that
would exceed the cap.
---
Outside diff comments:
In `@src/components/compose-message/hooks/useAttachments.test.ts`:
- Around line 35-50: Move the vi.restoreAllMocks() call from the afterEach block
into the beforeEach block so mocks are restored at test setup; specifically, in
the test file adjust the lifecycle hooks so beforeEach calls
vi.restoreAllMocks() before
run.mockReset()/retry.mockReset()/remove.mockReset()/clear.mockReset()/show.mockReset()
and the afterEach no longer calls vi.restoreAllMocks(); update references in the
setup where run.mockImplementation and lastCallbacks are initialized to run
after the restore.
- Line 3: Update the internal import in useAttachments.test.ts to use the
project alias instead of a relative path: replace import useAttachments from
'./useAttachments' with the aliased path import useAttachments from
'`@/components/compose-message/hooks/useAttachments`' so it follows the `@/*` →
`src/*` convention and resolves correctly across the codebase.
🪄 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: bc5935ca-73a3-4d04-b323-14edc720cea2
📒 Files selected for processing (2)
src/components/compose-message/hooks/useAttachments.test.tssrc/components/compose-message/hooks/useAttachments.ts
|
2 participants
Custom hook that handles the file attachments.