[PB-1967]: feat/upload manager#58
Conversation
|
Warning Review limit reached
More reviews will be available in 50 minutes and 3 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. 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 (1)
📝 WalkthroughWalkthroughAdds MailService attachment upload/download methods, defines upload manager types, implements a concurrent UploadManager with retry and cancellation, enables global HTTP retry, updates async dependencies, and adds Vitest tests covering upload flows. ChangesAttachment Upload and Task Management
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
Deploying mail-web with
|
| Latest commit: |
eae4f68
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://1436f761.mail-web-ea0.pages.dev |
| Branch Preview URL: | https://feature-upload-attachment-ma.mail-web-ea0.pages.dev |
![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: 5
🤖 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/services/upload-manager/index.ts`:
- Around line 68-82: In uploadWithRetries, don't blindly retry every rejection
from MailService.instance.uploadAttachment; instead detect transient failures
(e.g., network timeouts, 5xx responses) and only loop when
isTransientError(error) is true (or error.statusCode/ error.code indicates
transient), otherwise rethrow immediately; alternatively add an idempotencyToken
property to UploadAttachmentTask and pass it into
MailService.instance.uploadAttachment(task.file, { idempotencyToken }) so the
server can safely make retries idempotent; update the retry loop in
uploadWithRetries to consult the transient check and the task.cancelled flag,
only assign/clear task.canceler around attempts, and throw the lastError only if
it was transient and max retries exhausted.
- Around line 34-54: Add JSDoc comments to the public service methods run,
retry, and remove to document their purpose and contract: for run describe that
it accepts an array of File and UploadAttachmentCallbacks, returns an array of
UploadHandle objects with id and file, and that it enqueues tasks via
enqueueTask; for retry describe that it takes an existing task id and callbacks,
cancels the current task if present and re-enqueues it; for remove describe that
it cancels and deletes the task by id. Include `@param` and `@returns` tags (and any
thrown/side-effect notes) and reference the UploadAttachmentCallbacks and
UploadHandle types in the descriptions.
In `@src/services/upload-manager/upload-manager.test.ts`:
- Line 2: The test imports the internal module using a relative path
("./index"); update the import to use the project path alias so it follows the
src/* convention—replace the './index' import for UploadManager in
upload-manager.test.ts with the '`@/services/upload-manager`' style import
(referencing the UploadManager symbol) so the test uses the alias-based module
path.
- Around line 29-31: Replace the current beforeEach that only calls
uploadAttachment.mockReset() with a call to vi.restoreAllMocks() so all
spies/mocks are restored between tests; locate the beforeEach block in this test
file (the one referencing uploadAttachment) and change it to invoke
vi.restoreAllMocks() (you may keep or add uploadAttachment.mockReset() if you
still need to clear call history, but ensure vi.restoreAllMocks() is called to
fully restore mocked implementations).
- Line 38: Rename the test titles to describe observable behavior using the
"When <situation>, then <expected outcome>" pattern and remove
implementation-specific terms like "id", "File", "onSuccess", and "canceler";
for example change "When files are pushed, then it returns a handle per file
with stable id and File" to something like "When files are pushed, then each
pushed file yields a stable handle" and similarly update the other titles that
mention implementation details (the test titled at "When files are pushed..."
and the tests around the strings at the other two occurrences) so they focus
only on what a user/system observes rather than internal variable or type names.
🪄 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: 08eda9bf-cda1-4e8b-99e2-d650925170c3
📒 Files selected for processing (4)
src/services/sdk/mail/index.tssrc/services/upload-manager/index.tssrc/services/upload-manager/upload-manager.test.tssrc/types/mail/upload-manager/index.ts
| if (!isTransientError(error)) throw error; | ||
| } finally { | ||
| task.canceler = undefined; | ||
| } | ||
| } | ||
| throw lastError; | ||
| } | ||
| } | ||
|
|
||
| function isTransientError(error: unknown): boolean { | ||
| if (error instanceof AxiosResponseError) return error.status >= 500; | ||
| if (error instanceof AxiosUnknownError) { | ||
| return error.code === 'ECONNABORTED' || error.code === 'ETIMEDOUT' || error.code === 'ERR_NETWORK'; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
seems like this misses 429s. those should also be retryable and even better if we honor Retry-After header if included in response
There was a problem hiding this comment.
Even better, the SDK has a method for that. Just by calling HttpClient.enableGlobalRetry, the SDK will manage the retry back off for each SDK call internally.
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 (1)
src/services/upload-manager/index.ts (1)
86-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop canceled tasks before starting the next retry attempt.
A task can be canceled after a transient failure and before the next loop iteration, but this code still calls
MailService.instance.uploadAttachment(task.file)again on Line 89. For uploads, that meansremove()/retry()can still trigger one extra write after the user has canceled it.Suggested fix
private async uploadWithRetries(task: UploadAttachmentTask): Promise<UploadAttachmentResponse> { let lastError: unknown; for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) { + if (task.cancelled) throw new Error(CANCEL_REASON); + const { promise, requestCanceler } = MailService.instance.uploadAttachment(task.file); task.canceler = requestCanceler; try { return await promise;🤖 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 86 - 96, The retry loop in uploadWithRetries calls MailService.instance.uploadAttachment(task.file) even if the task was canceled between attempts; to fix, ensure you check task.cancelled before starting a new attempt (either at the top of the for-loop or immediately after obtaining/assigning requestCanceler) and if canceled, cancel the pending request (call the requestCanceler if available) and throw/return a cancellation error (or rethrow lastError) instead of proceeding; update the logic in uploadWithRetries (referencing MAX_RETRIES, task.canceler, task.cancelled, and MailService.instance.uploadAttachment) so no new upload is started for a task that has been marked cancelled.
🤖 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 dependency uses unstable deep imports from `@internxt/sdk/dist/`*
which can break on minor updates; either pin the package to the exact working
version or remove deep imports and switch to the SDK's public API: update
package.json to replace "^1.17.4" with the exact resolved version (e.g.,
"1.17.4") if you want to pin, or refactor all imports in files such as
src/services/sdk/index.ts and src/services/upload-manager/index.ts (and any
other files importing `@internxt/sdk/dist/`*) to use the SDK's documented public
entrypoints, then run install/build/tests to verify no import errors remain.
---
Outside diff comments:
In `@src/services/upload-manager/index.ts`:
- Around line 86-96: The retry loop in uploadWithRetries calls
MailService.instance.uploadAttachment(task.file) even if the task was canceled
between attempts; to fix, ensure you check task.cancelled before starting a new
attempt (either at the top of the for-loop or immediately after
obtaining/assigning requestCanceler) and if canceled, cancel the pending request
(call the requestCanceler if available) and throw/return a cancellation error
(or rethrow lastError) instead of proceeding; update the logic in
uploadWithRetries (referencing MAX_RETRIES, task.canceler, task.cancelled, and
MailService.instance.uploadAttachment) so no new upload is started for a task
that has been marked cancelled.
🪄 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: 4ae85379-1b3f-4e0f-ba0b-389f20ea4991
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/services/sdk/index.tssrc/services/upload-manager/index.tssrc/services/upload-manager/upload-manager.test.ts
| "@internxt/css-config": "^1.1.0", | ||
| "@internxt/lib": "^1.4.1", | ||
| "@internxt/sdk": "1.17.2", | ||
| "@internxt/sdk": "^1.17.4", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'SDK version in package.json:\n'
jq -r '.dependencies["`@internxt/sdk`"]' package.json
printf '\nDeep SDK imports currently used in src/:\n'
rg -n "`@internxt/sdk/dist/`" srcRepository: internxt/mail-web
Length of output: 6977
Pin @internxt/sdk (or stop relying on dist/* deep imports)
package.json sets @internxt/sdk to ^1.17.4, but src/ imports multiple internal SDK artifacts via @internxt/sdk/dist/* (e.g., src/services/sdk/index.ts and src/services/upload-manager/index.ts, plus many other files). Because dist/* paths aren’t a stable public API, an SDK refresh can break builds without any code changes.
Pin @internxt/sdk to the exact version used by these imports, or refactor to SDK public exports/entrypoints.
🤖 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` at line 27, The dependency uses unstable deep imports from
`@internxt/sdk/dist/`* which can break on minor updates; either pin the package to
the exact working version or remove deep imports and switch to the SDK's public
API: update package.json to replace "^1.17.4" with the exact resolved version
(e.g., "1.17.4") if you want to pin, or refactor all imports in files such as
src/services/sdk/index.ts and src/services/upload-manager/index.ts (and any
other files importing `@internxt/sdk/dist/`*) to use the SDK's documented public
entrypoints, then run install/build/tests to verify no import errors remain.
|
2 participants
Adding an upload manager to handle the attachment upload.