feat: implement resumable uploads with pause, resume, and cancel#39635
Open
AayushKrGupta wants to merge 5 commits into
Open
feat: implement resumable uploads with pause, resume, and cancel#39635AayushKrGupta wants to merge 5 commits into
AayushKrGupta wants to merge 5 commits into
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 55b9b78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds resumable, chunked uploads: new server endpoints (init/chunk/complete/cancel) with per-upload temp sessions and filesystem storage, client-side pause/resume/cancel in the UploadsStore and API, UI wiring for pause/resume controls, and type additions for resumable state and upload status. Changes
Sequence DiagramsequenceDiagram
actor User
participant Composer as Message Composer
participant Store as Upload Store
participant API as Client API
participant Server as Upload Server
participant FS as File System
User->>Composer: Select & upload file
Composer->>Store: send(file)
Store->>API: POST /uploads/init
API->>Server: /uploads/init
Server->>FS: Create temp session dir
Server-->>API: { uploadId, chunkSize, totalChunks }
loop For each chunk
Note over Store: Check paused/cancel state
alt Paused
Store->>Store: Wait for resume event
end
Store->>API: POST /uploads/chunk (uploadId, index, data)
API->>Server: /uploads/chunk
Server->>FS: Store chunk file
Server-->>API: { success: true }
API-->>Store: Update progress
Store->>Composer: Emit update
end
alt Cancelled
Composer->>Store: cancel(id)
Store->>API: POST /uploads/cancel
API->>Server: /uploads/cancel
Server->>FS: Remove temp session
else Complete
Store->>API: POST /uploads/complete
API->>Server: /uploads/complete
Server->>FS: Merge chunks → final file
Server->>Server: Store via FileUpload
Server-->>API: { file }
API-->>Store: Return file metadata
Store->>Composer: Emit update (status=complete)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/lib/chats/uploads.ts">
<violation number="1" location="apps/meteor/client/lib/chats/uploads.ts:224">
P2: Failed uploads are not cancelled on the server, so temporary resumable-upload directories and session records can be orphaned when chunk upload or completion fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
162-171:⚠️ Potential issue | 🔴 CriticalMissing destructuring for
handlePauseUploadandhandleResumeUpload.The handlers are used on lines 460-461 but are not destructured from
useFileUpload, causingundefinedto be passed as props.🐛 Proposed fix to add missing destructuring
const { uploads, hasUploads, handleUploadFiles, handleEditUpload, handleRemoveUpload, handleCancelUpload, + handlePauseUpload, + handleResumeUpload, isUploading, isProcessingUploads, } = useFileUpload(uploadsStore);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx` around lines 162 - 171, The destructuring from useFileUpload is missing handlePauseUpload and handleResumeUpload, causing undefined handlers to be passed; update the destructuring assignment where useFileUpload(uploadsStore) is called (the const { ... } = useFileUpload(uploadsStore) block) to include handlePauseUpload and handleResumeUpload so the MessageBox component passes valid functions to whichever components use these handlers.
🧹 Nitpick comments (3)
apps/meteor/app/api/server/v1/uploads.ts (2)
140-145: Add type definitions for request body parameters.Using
as anybypasses TypeScript's type checking. Define proper request body schemas to ensure type safety and validation.♻️ Proposed fix to add type definitions
+type UploadsInitParams = { + rid: string; + fileName: string; + fileSize: number; + fileType: string; + chunkSize: number; +}; + async function action() { - const { rid, fileName, fileSize, fileType, chunkSize } = this.bodyParams as any; + const { rid, fileName, fileSize, fileType, chunkSize } = this.bodyParams as UploadsInitParams;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 140 - 145, Replace the unsafe cast "this.bodyParams as any" in the action function by creating a specific request type (e.g., UploadInitRequest with rid, fileName, fileSize, fileType, chunkSize) and use it to type this.bodyParams (or annotate a local const typed as UploadInitRequest); also add a lightweight runtime validation/type-guard (or use the existing validation library in the project) to verify each field's type/shape before proceeding so the current null-checks become type-safe and TypeScript can catch misuse of rid, fileName, fileSize, fileType, and chunkSize.
331-338: Inconsistent use of synchronousfs.existsSyncin async handler.This mixes sync and async filesystem operations. Also, the directory existence check before cleanup introduces a TOCTOU race condition.
♻️ Proposed fix to use async-only operations
const sessionDir = path.join(TEMP_UPLOAD_DIR, uploadId); - if (fs.existsSync(sessionDir)) { - const chunkFiles = await readdir(sessionDir); + try { + const chunkFiles = await readdir(sessionDir); for (const chunkFile of chunkFiles) { await unlink(path.join(sessionDir, chunkFile)); } await rmdir(sessionDir); + } catch (e) { + if ((e as NodeJS.ErrnoException).code !== 'ENOENT') { + throw e; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 331 - 338, Remove the synchronous fs.existsSync check and instead attempt the async cleanup directly: build sessionDir (path.join(TEMP_UPLOAD_DIR, uploadId)), call readdir(sessionDir) inside a try/catch, iterate chunkFiles and await unlink(path.join(sessionDir, chunkFile)), then await rmdir(sessionDir); if readdir or rmdir throws ENOENT, ignore it (directory already gone) and rethrow other errors; this eliminates the TOCTOU race and keeps all operations async (references: sessionDir, TEMP_UPLOAD_DIR, uploadId, readdir, unlink, rmdir, fs.existsSync).apps/meteor/client/lib/chats/uploads.ts (1)
129-129: Drop the inline phase comments.
CHUNK_SIZE, the pause branch, and thecompletecall already make these phases clear without narration. As per coding guidelines, "Avoid code comments in the implementation."Also applies to: 144-144, 169-169, 220-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/chats/uploads.ts` at line 129, Remove the inline phase comments around the upload implementation: delete the comment lines that narrate the chunking, pause branch, and completion phases — the constants and code already communicate these steps; specifically remove comments adjacent to CHUNK_SIZE, the pause branch (the code handling paused uploads), and the call to complete (e.g., completeUpload/complete()) in apps/meteor/client/lib/chats/uploads.ts so the implementation contains no explanatory inline comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 289-294: The current flow calls fileStore.insert(details, file)
then performs cleanup (await unlink(finalPath); await rmdir(sessionDir); await
Uploads.deleteOne({ _id: uploadId })), which can leave the Uploads session
record orphaned if cleanup throws; modify the flow to ensure the session record
is removed regardless of cleanup success by either deleting the Uploads record
before calling fileStore.insert or by wrapping the insert+cleanup in a
try/catch/finally that guarantees Uploads.deleteOne({ _id: uploadId }) runs on
both success and failure (and in the catch, attempt any necessary rollback like
removing the stored file if insert succeeded); reference functions/variables
fileStore.insert, unlink, rmdir, Uploads.deleteOne, uploadId, sessionDir, and
finalPath when making the change.
- Around line 35-41: The ensureTempDir function currently swallows all errors
from mkdir(TEMP_UPLOAD_DIR), which can hide permissions or disk I/O failures;
update ensureTempDir to surface failures by either rethrowing the caught error
or logging it and then throwing a new Error with context (include
TEMP_UPLOAD_DIR in the message) so callers can react to filesystem problems
rather than proceeding silently; ensure the change preserves async behavior and
references ensureTempDir and TEMP_UPLOAD_DIR so the failing mkdir error is not
silently ignored.
- Around line 266-275: The merge loop using writeStream in uploads.ts doesn't
handle write errors or backpressure; replace the manual write loop with a proper
streaming pattern (e.g., use stream.pipeline or the async pipeline from
'stream/promises' to pipe chunk read streams into the final write stream) or
explicitly await drain and attach 'error' handlers on writeStream so any write
error rejects the operation; ensure you reference and update the merge logic
that uses writeStream, chunkFiles, sessionDir and finalPath, and make the
overall merge awaitable so failures propagate instead of silently continuing.
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 145-164: The cancel handler for uploads is attached too late
(after awaiting sdk.rest.post('/v1/uploads/init')), causing cancellations during
init to be dropped and leaving multiple per-chunk handlers attached; to fix,
register a single cancellation listener for `cancelling-${id}` before calling
sdk.rest.post('/v1/uploads/init') (store the handler reference so you can remove
it), reuse that handler to abort the active XHR(s) and call
sdk.rest.post('/v1/uploads/cancel') with the returned uploadId, and ensure you
call this.off(`cancelling-${id}`, handler) in a finally block (or after
success/failure) to clean up; update any per-chunk logic (the code around
CHUNK_SIZE and the per-chunk handlers) to avoid adding new once() listeners and
instead rely on the single shared handler to cancel the upload and remove the
upload from this.uploads.
- Around line 223-225: The catch block in the upload flow is marking cancelled
uploads as status: 'error'; modify the handler in the upload method surrounding
updateUpload so it first checks the upload's cancelled flag (e.g.,
this.uploads[id]?.cancelled or a local cancelled variable) and returns early if
true, otherwise proceed to call updateUpload(id, { percentage: 0, error: new
Error(getErrorMessage(error)), status: 'error' }); ensure you reference the
existing updateUpload and getErrorMessage symbols and do not alter other cleanup
logic that runs on cancel.
---
Outside diff comments:
In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx`:
- Around line 162-171: The destructuring from useFileUpload is missing
handlePauseUpload and handleResumeUpload, causing undefined handlers to be
passed; update the destructuring assignment where useFileUpload(uploadsStore) is
called (the const { ... } = useFileUpload(uploadsStore) block) to include
handlePauseUpload and handleResumeUpload so the MessageBox component passes
valid functions to whichever components use these handlers.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 140-145: Replace the unsafe cast "this.bodyParams as any" in the
action function by creating a specific request type (e.g., UploadInitRequest
with rid, fileName, fileSize, fileType, chunkSize) and use it to type
this.bodyParams (or annotate a local const typed as UploadInitRequest); also add
a lightweight runtime validation/type-guard (or use the existing validation
library in the project) to verify each field's type/shape before proceeding so
the current null-checks become type-safe and TypeScript can catch misuse of rid,
fileName, fileSize, fileType, and chunkSize.
- Around line 331-338: Remove the synchronous fs.existsSync check and instead
attempt the async cleanup directly: build sessionDir (path.join(TEMP_UPLOAD_DIR,
uploadId)), call readdir(sessionDir) inside a try/catch, iterate chunkFiles and
await unlink(path.join(sessionDir, chunkFile)), then await rmdir(sessionDir); if
readdir or rmdir throws ENOENT, ignore it (directory already gone) and rethrow
other errors; this eliminates the TOCTOU race and keeps all operations async
(references: sessionDir, TEMP_UPLOAD_DIR, uploadId, readdir, unlink, rmdir,
fs.existsSync).
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Line 129: Remove the inline phase comments around the upload implementation:
delete the comment lines that narrate the chunking, pause branch, and completion
phases — the constants and code already communicate these steps; specifically
remove comments adjacent to CHUNK_SIZE, the pause branch (the code handling
paused uploads), and the call to complete (e.g., completeUpload/complete()) in
apps/meteor/client/lib/chats/uploads.ts so the implementation contains no
explanatory inline comments.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ef5a808-4a64-4ca3-aed8-014b0e6e44ec
📒 Files selected for processing (9)
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsxpackages/core-typings/src/IUpload.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tspackages/core-typings/src/IUpload.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsxapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
📚 Learning: 2026-03-12T17:12:42.624Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsxapps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsxapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tsapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tspackages/core-typings/src/IUpload.tsapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/lib/chats/ChatAPI.tsapps/meteor/client/lib/chats/Upload.tspackages/core-typings/src/IUpload.tsapps/meteor/client/views/room/body/hooks/useFileUpload.tsapps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsx
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsx
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
🔇 Additional comments (6)
packages/core-typings/src/IUpload.ts (1)
67-72: LGTM!The new
resumableproperty is well-structured for tracking multipart upload state. The optional nature ensures backward compatibility with existing uploads.apps/meteor/client/lib/chats/Upload.ts (1)
9-9: LGTM!The
statusfield addition is well-typed and enables proper UI state reflection for the pause/resume workflow. SinceEncryptedUploadextendsNonEncryptedUpload, the field is consistently available across upload types.apps/meteor/client/views/room/body/hooks/useFileUpload.ts (1)
45-48: LGTM!The new pause and resume handlers follow the same pattern as existing handlers and are properly memoized with the correct dependency array.
apps/meteor/client/lib/chats/ChatAPI.ts (1)
117-118: LGTM!The new
pauseandresumemethod signatures are consistent with the existingcancelmethod and properly typed.apps/meteor/client/views/room/composer/messageBox/MessageComposerFiles.tsx (1)
12-13: LGTM!The new
onPauseandonResumeprops are properly typed and passed through to child components, maintaining consistency with the existing callback pattern.Also applies to: 17-17, 28-29
apps/meteor/client/views/room/composer/messageBox/MessageComposerFileItem.tsx (1)
67-78: LGTM!The pause/resume UI logic correctly handles the upload status. When
statusis undefined during an active upload, the pause button is appropriately shown as the default. TheBoxwrapper for layout and use ofusePreventPropagationfor handlers are clean implementations.
Comment on lines
+145
to
+164
| const { uploadId } = (await sdk.rest.post('/v1/uploads/init', { | ||
| rid: this.rid, | ||
| fileName: file.name, | ||
| fileSize: file.size, | ||
| fileType: file.type, | ||
| chunkSize: CHUNK_SIZE, | ||
| })) as any; | ||
|
|
||
| const totalChunks = Math.ceil(file.size / CHUNK_SIZE); | ||
| let cancelled = false; | ||
|
|
||
| this.once(`cancelling-${id}`, async () => { | ||
| cancelled = true; | ||
| try { | ||
| await sdk.rest.post('/v1/uploads/cancel', { uploadId }); | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| this.set(this.uploads.filter((upload) => upload.id !== id)); | ||
| }); |
Contributor
There was a problem hiding this comment.
Attach cancellation before init and clean it up once.
The first cancelling-${id} handler is only attached on Line 156, so a cancel fired while /v1/uploads/init is in flight is dropped and the upload continues. Then Lines 209-212 add another handler for every chunk and never remove the successful ones, which leaves one stale closure per uploaded chunk on the store instance. Register a single cancel handler before the first awaited request, reuse it to abort the active XHR, and off() it in finally.
Also applies to: 194-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/chats/uploads.ts` around lines 145 - 164, The cancel
handler for uploads is attached too late (after awaiting
sdk.rest.post('/v1/uploads/init')), causing cancellations during init to be
dropped and leaving multiple per-chunk handlers attached; to fix, register a
single cancellation listener for `cancelling-${id}` before calling
sdk.rest.post('/v1/uploads/init') (store the handler reference so you can remove
it), reuse that handler to abort the active XHR(s) and call
sdk.rest.post('/v1/uploads/cancel') with the returned uploadId, and ensure you
call this.off(`cancelling-${id}`, handler) in a finally block (or after
success/failure) to clean up; update any per-chunk logic (the code around
CHUNK_SIZE and the per-chunk handlers) to avoid adding new once() listeners and
instead rely on the single shared handler to cancel the upload and remove the
upload from this.uploads.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/resumable-uploads-support.md:
- Around line 2-3: Update the changeset entries for the two package bumps by
changing their release level from "patch" to "minor": modify the lines
containing "@rocket.chat/meteor": patch and "@rocket.chat/core-typings": patch
so they read "@rocket.chat/meteor": minor and "@rocket.chat/core-typings": minor
to reflect the added upload lifecycle endpoints and new resumable typing fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29803c9e-141a-4a11-96a1-0e61ed577d86
📒 Files selected for processing (1)
.changeset/resumable-uploads-support.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-12T17:12:42.624Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
.changeset/resumable-uploads-support.md
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/lib/chats/uploads.ts">
<violation number="1" location="apps/meteor/client/lib/chats/uploads.ts:175">
P2: Cancel during in-flight init can orphan resumable upload sessions because `uploadId` is discarded before server cancellation can run.</violation>
<violation number="2" location="apps/meteor/client/lib/chats/uploads.ts:219">
P1: Cancelling an in-flight chunk upload can leave the awaited Promise pending because aborted XHRs are not handled (`onabort` missing).</violation>
</file>
<file name="apps/meteor/app/api/server/v1/uploads.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/uploads.ts:321">
P1: `uploads/complete` now always deletes resumable session state in `finally`, even when final persistence fails, causing unrecoverable upload loss for that session.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
apps/meteor/client/lib/chats/uploads.ts (1)
129-129: Drop the inline implementation comments.These comments just restate the surrounding code.
As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 166-166, 181-181, 227-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/chats/uploads.ts` at line 129, Remove the inline implementation comments that simply restate code (e.g., the "// 1MB" comment after the CHUNK_SIZE constant in uploads.ts and the similar comments at the other indicated locations), leaving the code self-explanatory; locate the CHUNK_SIZE constant and the comment occurrences around the symbols referenced at the other noted lines (166, 181, 227) and delete those inline comments so only necessary documentation or descriptive comments remain.apps/meteor/app/api/server/v1/uploads.ts (1)
269-269: Drop the inline implementation comments.These comments just restate the surrounding code.
As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 294-294, 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` at line 269, Remove the redundant inline implementation comments (e.g., the "// Merge chunks" comment at the location shown and the similar comments at lines 294 and 310) from apps/meteor/app/api/server/v1/uploads.ts; locate the comment strings in the upload-related handler or function surrounding the chunk-merge and related logic and simply delete those lines so the code reads without restating the implementation (leave only high-level or documentation comments if truly necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 214-223: The handler currently accepts uploaded chunks via
MultipartUploadHandler.parseRequest without a maxSize and trusts caller-supplied
session.size when persisting metadata; fix by passing a maxSize equal to the
negotiated chunkSize (from the Upload document/session) to
MultipartUploadHandler.parseRequest, then validate that the received chunk file
size does not exceed that chunkSize and that the cumulative size of stored chunk
files plus this chunk does not exceed the agreed total (session.size stored
server-side); after moving the temp file to
TEMP_UPLOAD_DIR/<uploadId>/<chunkIndex>, recompute the merged byte length from
existing chunk files (do not trust client session.size) and persist that
recomputed size when updating Uploads (e.g., in Uploads.updateOne and when
updating 'resumable.uploadedChunks'), rejecting the request if any size checks
fail.
- Around line 143-178: Before creating or completing the resumable upload
session, verify the caller is authorized to upload to the target room id (rid);
specifically, call the existing room authorization helper (e.g., lookup the room
via Rooms.findOneById / room authorization function or the project's
room-permission helper) and ensure the current user has upload/access rights for
that room, then proceed to Uploads.updateOne (in action) and the completion
handlers (the resumable finalize logic referenced around the 247-322 handlers).
If the authorization fails, return an API.v1.failure error and do not create
temp dirs or write any upload metadata or files (i.e., block before
ensureTempDir / mkdir and before completing chunks).
- Around line 295-308: The code currently re-reads the merged file into memory
with readFile(finalPath) before calling FileUpload.getStore('Uploads').insert,
which causes unnecessary memory bloat; instead, avoid re-buffering by handing
off the on-disk temp file (e.g. pass a fs.createReadStream(finalPath) or the
finalPath itself if the store supports file-path ingestion) to fileStore.insert
(or use a fileStore.insertFromPath helper) so the upload is streamed from disk
into the store without copying the whole file into memory; update the call site
around readFile, fileStore, uploadedFile and ensure any cleanup still runs after
the stream finishes.
- Around line 203-223: The code currently uses the raw query value chunkIndex
when building chunkPath allowing path traversal; validate and normalize
chunkIndex before using it (e.g. ensure it's present and a non-negative
integer), convert it to a safe filename (e.g. const safeIndex =
String(Number(chunkIndex)) after checking Number.isInteger and >=0) and then
join with TEMP_UPLOAD_DIR and uploadId when calling rename; additionally, after
computing the target path (used in rename) verify it resides under the intended
session directory (using path.resolve and a startsWith check against the
resolved session dir) before moving the temp file and before updating Uploads
with the sanitized Number(chunkIndex).
- Around line 354-368: The cancel handler currently deletes any Uploads document
owned by the caller, allowing completed uploads to be removed; after retrieving
the session with Uploads.findOneById, add a guard that ensures the document
represents a resumable/chunked upload session (e.g. check a flag like
session.isResumable === true or session.type === 'resumable' or
session.completed !== true) and if it is not a resumable session return
API.v1.notFound() without deleting; only when the session passes that resumable
check proceed to remove the TEMP_UPLOAD_DIR/sessionDir chunks and call
Uploads.deleteOne({ _id: uploadId }) so completed uploads follow their normal
cleanup path.
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 167-176: Assign and persist the uploadId immediately after
receiving initResponse (i.e., set uploadId = initResponse.uploadId right after
the sdk.rest.post('/v1/uploads/init') resolves) before the cancelled check so
the server-side session/temp dir can be referenced/cancelled later; ensure
uploadId is declared in the enclosing scope where cancel logic runs (referencing
the variables uploadId, initResponse, cancelled and the REST call to
'/v1/uploads/init') and then keep the existing early return for cancelled.
- Around line 204-220: The chunk upload promise only hooks currentXhr.onload and
currentXhr.onerror, so calling xhr.abort() from cancelHandler leaves the promise
unresolved; add a currentXhr.onabort (or attach
currentXhr.addEventListener('abort', ...)) that rejects the promise with a clear
AbortError (or same error flow used elsewhere), and ensure any existing
onload/onerror handlers are cleaned up if needed; locate the upload call via
sdk.rest.upload(...) and the surrounding promise where xhr, uploadId, chunkIndex
and cancelHandler are used and add the abort handler to settle the awaiting
promise when abort() is invoked.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Line 269: Remove the redundant inline implementation comments (e.g., the "//
Merge chunks" comment at the location shown and the similar comments at lines
294 and 310) from apps/meteor/app/api/server/v1/uploads.ts; locate the comment
strings in the upload-related handler or function surrounding the chunk-merge
and related logic and simply delete those lines so the code reads without
restating the implementation (leave only high-level or documentation comments if
truly necessary).
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Line 129: Remove the inline implementation comments that simply restate code
(e.g., the "// 1MB" comment after the CHUNK_SIZE constant in uploads.ts and the
similar comments at the other indicated locations), leaving the code
self-explanatory; locate the CHUNK_SIZE constant and the comment occurrences
around the symbols referenced at the other noted lines (166, 181, 227) and
delete those inline comments so only necessary documentation or descriptive
comments remain.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d8f0887-27ca-4503-b628-1cdf52b91ec1
📒 Files selected for processing (3)
.changeset/resumable-uploads-support.mdapps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/resumable-uploads-support.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-12T17:12:42.624Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/uploads.ts
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/uploads.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/uploads.ts:305">
P1: Cleanup and session deletion no longer run when `fileStore.insert` fails, causing leaked temp files and stale resumable-upload records.</violation>
<violation number="2" location="apps/meteor/app/api/server/v1/uploads.ts:313">
P1: `uploads/complete` is not idempotent under concurrent/retried calls; deferred chunk cleanup widens a race that can insert duplicate files.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await unlink(finalPath); | ||
| } | ||
| if (fs.existsSync(sessionDir)) { | ||
| for (const chunkFile of chunkFiles) { |
Contributor
There was a problem hiding this comment.
P1: uploads/complete is not idempotent under concurrent/retried calls; deferred chunk cleanup widens a race that can insert duplicate files.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/uploads.ts, line 313:
<comment>`uploads/complete` is not idempotent under concurrent/retried calls; deferred chunk cleanup widens a race that can insert duplicate files.</comment>
<file context>
@@ -303,23 +302,23 @@ const uploadsCompleteEndpoint = API.v1.post(
+ await unlink(finalPath);
+ }
+ if (fs.existsSync(sessionDir)) {
+ for (const chunkFile of chunkFiles) {
+ await unlink(path.join(sessionDir, chunkFile));
}
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/meteor/app/api/server/v1/uploads.ts (4)
143-177:⚠️ Potential issue | 🟠 MajorAuthorize
ridbefore reserving or storing bytes.Lines 157 and 297 trust the caller-provided room id without checking whether the user may upload to that room. That still lets any authenticated user create temp state and eventually persist an upload against an arbitrary
rid. Please run the same room/file-upload authorization as the normal upload path beforeUploads.updateOneand again beforefileStore.insert.Also applies to: 254-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 143 - 177, Before creating temp state or persisting bytes, validate that this.userId is allowed to upload to the provided rid: perform the same room/file-upload authorization used by the normal upload path (e.g., call the room access / upload-permission check function used elsewhere) inside action() and before any Uploads.updateOne call that reserves state and again right before the fileStore.insert call; if authorization fails, return API.v1.failure. Update action() to run that check using the same helper(s) (the existing room/upload permission method) to prevent trusting caller-provided rid.
214-223:⚠️ Potential issue | 🟠 MajorChunk count alone doesn't enforce the negotiated file size.
MultipartUploadHandler.parseRequestis still called without amaxSize, and Lines 265-266 only compare the number of files tototalChunks. A client can sendtotalChunksoversized chunks and Line 300 will still persist the caller-suppliedsession.size. Enforcesession.resumable.chunkSizeper chunk and derive the stored size from the bytes actually merged.Also applies to: 265-266, 296-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 214 - 223, The upload flow accepts chunks without enforcing per-chunk size and trusts client-provided session.size; update MultipartUploadHandler.parseRequest calls (where chunk is parsed) to pass a maxSize derived from session.resumable.chunkSize so each chunk is rejected if it exceeds the negotiated chunk size, validate chunk byte length after rename/move before adding to TEMP_UPLOAD_DIR, and when finalizing in the merge routine compute and persist the final size from the actual merged bytes (not session.size) and remove reliance on client-supplied total size; update code paths around MultipartUploadHandler.parseRequest, TEMP_UPLOAD_DIR chunk handling, Uploads.updateOne and the merge/finalize logic that sets session.size/session.resumable to use the computed byte count.
203-223:⚠️ Potential issue | 🔴 CriticalNormalize
chunkIndexonce and use the normalized value everywhere.Line 220 feeds the raw query string into
path.join, while Line 223 separately converts it withNumber(...). Inputs like../../foocan escape the session directory, and alternate spellings such as01can diverge between the filename and the stored chunk index. Parse a bounded integer, requiresession.resumable, and build both the path and the DB update from that normalized value only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 203 - 223, Normalize and validate chunkIndex once: require session.resumable exists, parse chunkIndex from this.queryParams (e.g., using Number and Number.isInteger), enforce bounds (>=0 and < session.resumable.totalChunks or a safe max), and reject invalid values; then use that single normalized numeric value when building chunkPath (path.join(TEMP_UPLOAD_DIR, uploadId, String(normalizedChunkIndex))) and when calling Uploads.updateOne({ _id: uploadId }, { $addToSet: { 'resumable.uploadedChunks': normalizedChunkIndex } }); keep MultipartUploadHandler.parseRequest usage and file handling the same but do not use the raw query string anywhere to avoid path traversal or mismatched representations.
353-367:⚠️ Potential issue | 🔴 Critical
/uploads/cancelneeds the same resumable guard as/uploads/complete.Line 354 only checks ownership, and Line 367 deletes whatever
Uploadsdocument matches that id. IfuploadIdpoints at a completed upload instead of a resumable session, this bypasses the normal file/message cleanup path and drops the DB record directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 353 - 367, The cancel handler currently only checks ownership and then deletes the Uploads document, which allows deleting completed uploads; update the handler (the code using Uploads.findOneById(uploadId), session, and Uploads.deleteOne) to enforce the same resumable guard used by the /uploads/complete flow: verify the session indicates a resumable upload (e.g., session.resumable or session.state === 'resumable') before proceeding, and if it is not resumable return the appropriate API response (e.g., API.v1.methodNotAllowed or API.v1.notFound) so completed uploads cannot be deleted via /uploads/cancel; keep the existing chunk-directory cleanup logic only for resumable sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 33-44: The resumable upload flow is node-local and will break in
LB setups because session creation and chunk I/O use TEMP_UPLOAD_DIR and
ensureTempDir on the handling instance while the Uploads document lacks an
instanceId; add instance-aware routing by adding an instanceId field to the
Uploads schema/collection (e.g., set instanceId when creating the Uploads
session), persist that instanceId with the Uploads record, and restore the
previous instance-proxy logic so requests for writeChunk/readChunk check
Uploads.instanceId and proxy the request to that instance when different; update
the codepaths that create the session directory (referenced by TEMP_UPLOAD_DIR
and ensureTempDir) and the chunk write/read handlers to use Uploads.instanceId
for routing so LB non-sticky deployments work correctly (alternatively, if you
prefer a different approach, move chunk storage/metadata to shared storage or
add docs requiring sticky sessions).
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 136-149: The cancelHandler currently removes the local upload even
if server-side finalization (/v1/uploads/complete) has started, causing
race/orphaned uploads; fix by introducing a per-upload finalizing flag set when
you invoke sdk.rest.post('/v1/uploads/complete') and check that flag at the top
of cancelHandler (and in any UI cancel/pause handlers) to no-op and disallow
cancellation once finalization began; specifically, update the code paths that
call sdk.rest.post('/v1/uploads/complete') to mark upload.finalizing = true
before the request and update cancelHandler to return early if upload.finalizing
is true (and avoid calling xhr.abort(), /v1/uploads/cancel, or removing via
this.set(this.uploads.filter(...))). Optionally, also ensure server-side
/v1/uploads/complete rejects requests for uploads already cancelled if you
control the API.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 143-177: Before creating temp state or persisting bytes, validate
that this.userId is allowed to upload to the provided rid: perform the same
room/file-upload authorization used by the normal upload path (e.g., call the
room access / upload-permission check function used elsewhere) inside action()
and before any Uploads.updateOne call that reserves state and again right before
the fileStore.insert call; if authorization fails, return API.v1.failure. Update
action() to run that check using the same helper(s) (the existing room/upload
permission method) to prevent trusting caller-provided rid.
- Around line 214-223: The upload flow accepts chunks without enforcing
per-chunk size and trusts client-provided session.size; update
MultipartUploadHandler.parseRequest calls (where chunk is parsed) to pass a
maxSize derived from session.resumable.chunkSize so each chunk is rejected if it
exceeds the negotiated chunk size, validate chunk byte length after rename/move
before adding to TEMP_UPLOAD_DIR, and when finalizing in the merge routine
compute and persist the final size from the actual merged bytes (not
session.size) and remove reliance on client-supplied total size; update code
paths around MultipartUploadHandler.parseRequest, TEMP_UPLOAD_DIR chunk
handling, Uploads.updateOne and the merge/finalize logic that sets
session.size/session.resumable to use the computed byte count.
- Around line 203-223: Normalize and validate chunkIndex once: require
session.resumable exists, parse chunkIndex from this.queryParams (e.g., using
Number and Number.isInteger), enforce bounds (>=0 and <
session.resumable.totalChunks or a safe max), and reject invalid values; then
use that single normalized numeric value when building chunkPath
(path.join(TEMP_UPLOAD_DIR, uploadId, String(normalizedChunkIndex))) and when
calling Uploads.updateOne({ _id: uploadId }, { $addToSet: {
'resumable.uploadedChunks': normalizedChunkIndex } }); keep
MultipartUploadHandler.parseRequest usage and file handling the same but do not
use the raw query string anywhere to avoid path traversal or mismatched
representations.
- Around line 353-367: The cancel handler currently only checks ownership and
then deletes the Uploads document, which allows deleting completed uploads;
update the handler (the code using Uploads.findOneById(uploadId), session, and
Uploads.deleteOne) to enforce the same resumable guard used by the
/uploads/complete flow: verify the session indicates a resumable upload (e.g.,
session.resumable or session.state === 'resumable') before proceeding, and if it
is not resumable return the appropriate API response (e.g.,
API.v1.methodNotAllowed or API.v1.notFound) so completed uploads cannot be
deleted via /uploads/cancel; keep the existing chunk-directory cleanup logic
only for resumable sessions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb9e0899-5a28-43a6-86e7-e85fe66fc5cb
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-03-12T17:12:42.624Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/lib/chats/uploads.tsapps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
Comment on lines
+33
to
+44
| const TEMP_UPLOAD_DIR = path.join(os.tmpdir(), 'rocketchat-resumable-uploads'); | ||
|
|
||
| const ensureTempDir = async () => { | ||
| try { | ||
| await mkdir(TEMP_UPLOAD_DIR, { recursive: true }); | ||
| } catch (e: any) { | ||
| if (e.code !== 'EEXIST') { | ||
| console.error(`Failed to create temporary upload directory: ${TEMP_UPLOAD_DIR}`, e); | ||
| throw new Error(`Failed to create temporary upload directory: ${e.message}`); | ||
| } | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the uploads.ts file to understand the implementation
head -n 400 apps/meteor/app/api/server/v1/uploads.ts | tail -n +30 | head -n 350Repository: RocketChat/Rocket.Chat
Length of output: 8697
🏁 Script executed:
# Check file size first
wc -l apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 110
🏁 Script executed:
# Look for session affinity or load balancing configuration
rg -i "sticky|affinity|session.*routing|loadbalanc" --type ts --type js -A 3 -B 3 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 5152
🏁 Script executed:
# Check if temp state is stored in database or local filesystem
rg "TEMP_UPLOAD_DIR|resumable\." apps/meteor/app/api/server/v1/uploads.ts -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 1599
🏁 Script executed:
# Look for any shared storage mechanisms (Redis, Mongo) for upload state
rg -i "redis|cluster|shared.*storage" apps/meteor/app/api/server/v1/uploads.ts -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check for Meteor's session/subscription management that might handle routing
rg "Meteor.subscribe|Meteor.call|DDP" apps/meteor/app/api/server/v1/uploads.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for load balancing or proxy configuration at app level
rg -i "proxyaddr|session.*cookie|sticky" apps/meteor/app/api/server/ -l | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's any documentation about deployment or session handling
find . -name "*.md" -type f | xargs grep -l "resumable\|session.*affinity\|load.?balanc" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 107
🏁 Script executed:
# Look for how other upload endpoints or file operations handle multi-instance deployments
rg -i "node.?local|instance.?affinity|sticky" apps/meteor/app/ -l | head -10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if Rocket.Chat uses any load balancing strategy or sticky sessions
rg "sticky|affinity" apps/meteor/ --type ts -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1458
🏁 Script executed:
# Read the changeset to understand the resumable uploads feature context
cat ./.changeset/resumable-uploads-support.mdRepository: RocketChat/Rocket.Chat
Length of output: 217
🏁 Script executed:
# Check if Rocket.Chat has deployment documentation mentioning sticky sessions
find . -name "README*" -o -name "DEPLOYMENT*" -o -name "*.config.*" | xargs grep -l "sticky\|session\|load.?balanc" 2>/dev/null | head -10Repository: RocketChat/Rocket.Chat
Length of output: 114
🏁 Script executed:
# Look for environment variables or config related to session handling
rg "SESSION|COOKIE|AFFINITY" apps/meteor/ --type ts --type tsx | head -20Repository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
# Check how the app is deployed and if there's information about multiple instances
rg -i "cluster|worker.*threads|pm2|docker" docker-compose.yml .env.example README.md 2>/dev/null | head -30Repository: RocketChat/Rocket.Chat
Length of output: 659
🏁 Script executed:
# Look for any shared storage backends that might be used for temporary files
rg -i "s3|minio|storage|redis" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's any documentation about session handling or sticky sessions in deployment guides
rg -i "sticky.*session|session.*sticky|affinity|load.?balanc" . --type md | head -30Repository: RocketChat/Rocket.Chat
Length of output: 2004
🏁 Script executed:
# Look for any comments in the codebase about multi-instance deployments
rg -i "multi.*instance|single.*instance|cluster" apps/meteor/app/api/server/ -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if other resumable/temporary operations in the codebase address this
rg -r "tmpdir|temporary.*dir|session.*dir" apps/meteor/app/api/server/ -B 3 -A 3Repository: RocketChat/Rocket.Chat
Length of output: 10336
🏁 Script executed:
# Look for how Meteor handles DDP connections and whether they're sticky by default
rg -i "connection|websocket|ddp" apps/meteor/app/api/server/v1/uploads.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there are any authentication/session cookies that might enforce stickiness
rg "loginToken|authToken|sessionId" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's any middleware or configuration that enforces sticky sessions
rg -i "cookie.*path|sessionid|affinity" apps/meteor/server/ -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 3179
🏁 Script executed:
# Look for any Meteor-specific session handling that might be sticky by default
rg "Meteor.default_connection\|connection\._socket\|WebSocket" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's documentation about required deployment configurations
find . -path ./node_modules -prune -o -name "*.md" -type f -exec grep -l "sticky.*session\|session.*affinity" {} \; 2>/dev/null | head -10Repository: RocketChat/Rocket.Chat
Length of output: 92
🏁 Script executed:
# Look at how other streaming/resumable operations are handled in the codebase
rg -i "resumable\|streaming.*upload\|chunked.*upload" apps/meteor/app/ --type ts -l | head -10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the current architecture assumption - does MongoDB Uploads collection store enough state to resume from any instance?
rg "Uploads\." apps/meteor/app/api/server/v1/uploads.ts -B 1 -A 3 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 1069
🏁 Script executed:
# Read the CHANGELOG to see what it says about sticky sessions
grep -A 5 -B 5 "sticky session" ./apps/meteor/CHANGELOG.md | head -50Repository: RocketChat/Rocket.Chat
Length of output: 2980
🏁 Script executed:
# Check if REST API endpoints are documented as requiring sticky sessions
rg -i "sticky.*rest|rest.*sticky|affinity.*endpoint" docs/ apps/meteor/ -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's any middleware that would handle routing based on uploadId or session
rg "uploadId" apps/meteor/server/ -l | head -10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look at how Meteor exposes REST APIs and if there's any session handling
find . -path ./node_modules -prune -o -name "*.ts" -type f -exec grep -l "API.v1" {} \; | head -5Repository: RocketChat/Rocket.Chat
Length of output: 3942
🏁 Script executed:
# Check if client-side code for uploads enforces any session/connection stickiness
find apps/meteor/client -name "*upload*" -type f | head -10Repository: RocketChat/Rocket.Chat
Length of output: 155
🏁 Script executed:
# Check the client-side upload handler to see if there's any session affinity
cat apps/meteor/client/lib/chats/flows/uploadFiles.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2270
🏁 Script executed:
# Look for any connection/session management in uploads
rg "connection\|session\|affinity" apps/meteor/client/lib/chats/flows/uploadFiles.ts apps/meteor/client/lib/chats/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check how the API request is made from client - does it use persistent connection?
rg "fetch\|axios\|http\|request" apps/meteor/client/lib/chats/flows/uploadFiles.ts -B 2 -A 5 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if REST API inherits connection from Meteor's DDP (which would be sticky)
rg "this\.request\|this\.userId\|authRequired" apps/meteor/app/api/server/v1/uploads.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for deployment documentation about multi-instance setups
find . -name "*.md" -type f -exec grep -l "multiple.*instance\|high.*availability\|cluster" {} \; 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 210
🏁 Script executed:
# Look for any documentation or comments about sticky sessions and resumable uploads
rg "sticky.*session\|session.*affinity" apps/meteor/ docs/ --type ts --type md -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there are any warnings or docs added alongside the resumable uploads feature
git log --oneline --all | grep -i "resumable\|upload" | head -10Repository: RocketChat/Rocket.Chat
Length of output: 849
🏁 Script executed:
# Verify if sticky sessions are documented as a general deployment requirement
rg -i "sticky.*session" docs/ apps/meteor/CHANGELOG.md apps/meteor/HISTORY.md -B 2 -A 2 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 2252
🏁 Script executed:
# Check if there's a deployment configuration that validates/enforces sticky sessions
rg -i "sticky\|affinity\|session.*route" apps/meteor/server/ --type ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the exact commit that added resumable uploads to see if there were discussions about stickiness
git show 73c9c4bfb3:apps/meteor/app/api/server/v1/uploads.ts 2>/dev/null | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2826
🏁 Script executed:
# Check if there's any PR discussion or comments about multi-instance deployments
git log --all --grep="resumable.*upload\|sticky.*session" --oneline | head -20Repository: RocketChat/Rocket.Chat
Length of output: 189
🏁 Script executed:
# Look for any infrastructure docs about load balancer/reverse proxy configuration
find . -name "*.md" -o -name "*.txt" | xargs grep -l "nginx\|apache\|haproxy\|load.?balanc" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 103
🏁 Script executed:
# Verify: are REST API requests in Rocket.Chat authenticated via cookies that would maintain stickiness?
rg "authToken\|loginToken\|cookie" apps/meteor/app/api/server/v1/uploads.ts apps/meteor/app/api/server/api.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# That "Proxy upload to correct instance" commit looks relevant - let's examine it
git show e7e0785116 --statRepository: RocketChat/Rocket.Chat
Length of output: 668
🏁 Script executed:
# Look at the full content of that proxy fix
git show e7e0785116Repository: RocketChat/Rocket.Chat
Length of output: 4076
🏁 Script executed:
# Check if this fix is in the current codebase
rg "Proxy.*upload\|proxy.*instance" apps/meteor/app/api/server/v1/ -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for any routing logic that might handle cross-instance uploads
rg -i "instance\|redirect\|proxy" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for InstanceStatus in the current codebase to see if proxying is implemented
rg "InstanceStatus" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if InstanceStatus is used anywhere in the new uploads or upload-related code
rg "InstanceStatus" apps/meteor/app/api/server/v1/ -lRepository: RocketChat/Rocket.Chat
Length of output: 154
🏁 Script executed:
# Look for the proxy/redirect logic in current uploads code
rg "proxy\|redirect\|instanceId" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if the Uploads model stores instanceId
rg "instanceId" apps/meteor/ --type ts -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 3848
🏁 Script executed:
# Verify if the new resumable uploads stores which instance created it
rg "uploadId\|resumable" apps/meteor/app/api/server/v1/uploads.ts | grep -A 3 "updateOne\|$set"Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if Uploads model is modified to track instanceId in the schema
rg "instanceId" apps/meteor/app/models/ -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 129
🏁 Script executed:
# Verify: does the new code store instanceId anywhere?
grep -n "instanceId" apps/meteor/app/api/server/v1/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the Uploads schema/model to see if it includes instanceId field
find apps/meteor -path "*/models/*" -name "*[Uu]pload*" -type f | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Verify if there's any middleware that proxies upload requests to correct instance
rg "proxy|handler.*upload" apps/meteor/server/ --type ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 2243
🏁 Script executed:
# Check if the learning about PR 38071 relates to this concern
git show ccd76942bf --statRepository: RocketChat/Rocket.Chat
Length of output: 1594
🏁 Script executed:
# Check if the old upload proxying code still exists or was removed
rg "InstanceStatus\|file.instanceId" apps/meteor/server/ --type ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look at how the previous upload endpoints handled this in the file-upload system
rg "instanceId" apps/meteor/app/file-upload/ --type ts -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's any middleware that routes upload requests based on instance
rg "upload|instance" apps/meteor/app/api/server/router.ts -B 2 -A 5 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Final verification: check if the old upload proxy mechanism still exists somewhere
find apps/meteor -name "*.ts" -type f | xargs grep -l "instanceId.*upload\|upload.*instanceId" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Verify: are there any tests for multi-instance upload scenarios?
rg -i "multi.*instance.*upload|instance.*routing.*upload" apps/meteor/tests/ --type ts -lRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the MultipartUploadHandler to see if it has any instance-awareness
cat apps/meteor/app/api/server/lib/MultipartUploadHandler.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2348
🏁 Script executed:
# Final check: confirm the old instance-aware upload code is no longer in use
git log --all --oneline --grep="proxy.*upload\|instance.*upload" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 150
🏁 Script executed:
# See if there's any comment or TODO about this in the current code
rg "TODO|FIXME|XXX|instance|proxy" apps/meteor/app/api/server/v1/uploads.ts | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Verify the Uploads document schema to see if instanceId field is present
rg "interface.*IUpload|type.*IUpload" packages/core-typings/src/ --type ts -A 20 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 2456
Resumable upload state is node-local and unaware of instance routing.
Lines 176-177 create the session directory on the handling instance, line 220 writes chunks to that same local filesystem, and lines 259-263 read from it. The Uploads document does not track which instance created the session (instanceId field is absent from the schema), so load-balanced deployments will route upload chunks to arbitrary instances, causing failures. Earlier uploads had instance-aware proxying that routed requests to the correct instance as a fallback for deployments without sticky sessions enabled; this safeguard is not present in the resumable implementation. Either:
- Add
instanceIdtracking to Uploads and restore instance-aware request routing, or - Move temporary chunk storage and session metadata to shared infrastructure (database or shared filesystem), or
- Explicitly document that resumable uploads require sticky sessions to be configured on the load balancer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 33 - 44, The resumable
upload flow is node-local and will break in LB setups because session creation
and chunk I/O use TEMP_UPLOAD_DIR and ensureTempDir on the handling instance
while the Uploads document lacks an instanceId; add instance-aware routing by
adding an instanceId field to the Uploads schema/collection (e.g., set
instanceId when creating the Uploads session), persist that instanceId with the
Uploads record, and restore the previous instance-proxy logic so requests for
writeChunk/readChunk check Uploads.instanceId and proxy the request to that
instance when different; update the codepaths that create the session directory
(referenced by TEMP_UPLOAD_DIR and ensureTempDir) and the chunk write/read
handlers to use Uploads.instanceId for routing so LB non-sticky deployments work
correctly (alternatively, if you prefer a different approach, move chunk
storage/metadata to shared storage or add docs requiring sticky sessions).
Comment on lines
+136
to
+149
| const cancelHandler = async () => { | ||
| cancelled = true; | ||
| if (xhr) { | ||
| xhr.abort(); | ||
| } | ||
| if (uploadId) { | ||
| try { | ||
| await sdk.rest.post('/v1/uploads/cancel', { uploadId }); | ||
| } catch (e) { | ||
| // ignore | ||
| } | ||
| } | ||
| this.set(this.uploads.filter((upload) => upload.id !== id)); | ||
| }; |
Contributor
There was a problem hiding this comment.
Cancel is not authoritative once finalization starts.
After Line 236 starts /v1/uploads/complete, the flow is outside the abortable XHR path. If the user cancels in that window, Line 148 removes the local item while /uploads/cancel races the server-side finalize, which can still store the file successfully and leave an orphaned upload behind. Disable pause/cancel once finalization begins, or have /uploads/complete refuse work after the session has been cancelled.
Also applies to: 236-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/chats/uploads.ts` around lines 136 - 149, The
cancelHandler currently removes the local upload even if server-side
finalization (/v1/uploads/complete) has started, causing race/orphaned uploads;
fix by introducing a per-upload finalizing flag set when you invoke
sdk.rest.post('/v1/uploads/complete') and check that flag at the top of
cancelHandler (and in any UI cancel/pause handlers) to no-op and disallow
cancellation once finalization began; specifically, update the code paths that
call sdk.rest.post('/v1/uploads/complete') to mark upload.finalizing = true
before the request and update cancelHandler to return early if upload.finalizing
is true (and avoid calling xhr.abort(), /v1/uploads/cancel, or removing via
this.set(this.uploads.filter(...))). Optionally, also ensure server-side
/v1/uploads/complete rejects requests for uploads already cancelled if you
control the API.
Contributor
There was a problem hiding this comment.
4 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/uploads.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/uploads.ts:238">
P1: Chunk storage relies on node-local filesystem (`TEMP_UPLOAD_DIR`), which breaks in multi-instance deployments. Rocket.Chat officially supports scaling with multiple instances behind a load balancer (Traefik), where sticky sessions are not guaranteed. The `init` endpoint creates the session directory on one instance, but subsequent `chunk` and `complete` requests may be routed to different instances that lack access to those files. Either store chunks in shared storage (MongoDB/GridFS or shared filesystem), track `instanceId` on the upload document and proxy requests to the originating instance (as done in earlier upload code), or document that sticky sessions are required.</violation>
<violation number="2" location="apps/meteor/app/api/server/v1/uploads.ts:247">
P2: Chunk uploads rescan and stat all existing chunks on every request, causing O(n^2) filesystem I/O for large uploads.</violation>
<violation number="3" location="apps/meteor/app/api/server/v1/uploads.ts:254">
P2: Resumable completion can accept truncated uploads because validation only checks byte overflow, not exact total size.</violation>
<violation number="4" location="apps/meteor/app/api/server/v1/uploads.ts:295">
P2: Race condition: once `/v1/uploads/complete` is in flight, a user-triggered cancel can race against finalization. The cancel handler will send `/v1/uploads/cancel` and remove the local UI item, but the server-side complete endpoint doesn't check whether the session has been cancelled before merging chunks and inserting into `FileUpload`. This can leave an orphaned file in storage. Either disable the cancel button on the client once finalization begins, or have the `/uploads/complete` endpoint atomically check for cancellation (e.g., verify `session.uploading === true`) before writing the final file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return API.v1.failure('Chunk size exceeds limit'); | ||
| } | ||
|
|
||
| const chunkPath = path.resolve(TEMP_UPLOAD_DIR, uploadId as string, String(normalizedChunkIndex)); |
Contributor
There was a problem hiding this comment.
P1: Chunk storage relies on node-local filesystem (TEMP_UPLOAD_DIR), which breaks in multi-instance deployments. Rocket.Chat officially supports scaling with multiple instances behind a load balancer (Traefik), where sticky sessions are not guaranteed. The init endpoint creates the session directory on one instance, but subsequent chunk and complete requests may be routed to different instances that lack access to those files. Either store chunks in shared storage (MongoDB/GridFS or shared filesystem), track instanceId on the upload document and proxy requests to the originating instance (as done in earlier upload code), or document that sticky sessions are required.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/uploads.ts, line 238:
<comment>Chunk storage relies on node-local filesystem (`TEMP_UPLOAD_DIR`), which breaks in multi-instance deployments. Rocket.Chat officially supports scaling with multiple instances behind a load balancer (Traefik), where sticky sessions are not guaranteed. The `init` endpoint creates the session directory on one instance, but subsequent `chunk` and `complete` requests may be routed to different instances that lack access to those files. Either store chunks in shared storage (MongoDB/GridFS or shared filesystem), track `instanceId` on the upload document and proxy requests to the originating instance (as done in earlier upload code), or document that sticky sessions are required.</comment>
<file context>
@@ -206,21 +211,52 @@ const uploadsChunkEndpoint = API.v1.post(
+ return API.v1.failure('Chunk size exceeds limit');
+ }
+
+ const chunkPath = path.resolve(TEMP_UPLOAD_DIR, uploadId as string, String(normalizedChunkIndex));
+ if (!chunkPath.startsWith(path.resolve(TEMP_UPLOAD_DIR, uploadId as string))) {
+ await MultipartUploadHandler.cleanup(file.tempFilePath);
</file context>
| cumulativeSize += chunkStat.size; | ||
| } | ||
|
|
||
| if (cumulativeSize > session.size) { |
Contributor
There was a problem hiding this comment.
P2: Resumable completion can accept truncated uploads because validation only checks byte overflow, not exact total size.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/uploads.ts, line 254:
<comment>Resumable completion can accept truncated uploads because validation only checks byte overflow, not exact total size.</comment>
<file context>
@@ -206,21 +211,52 @@ const uploadsChunkEndpoint = API.v1.post(
+ cumulativeSize += chunkStat.size;
+ }
+
+ if (cumulativeSize > session.size) {
+ await unlink(chunkPath);
+ return API.v1.failure('Cumulative chunk size exceeds total file size');
</file context>
| await rename(file.tempFilePath, chunkPath); | ||
|
|
||
| // Validate cumulative size | ||
| const chunkFiles = await readdir(path.join(TEMP_UPLOAD_DIR, uploadId as string)); |
Contributor
There was a problem hiding this comment.
P2: Chunk uploads rescan and stat all existing chunks on every request, causing O(n^2) filesystem I/O for large uploads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/uploads.ts, line 247:
<comment>Chunk uploads rescan and stat all existing chunks on every request, causing O(n^2) filesystem I/O for large uploads.</comment>
<file context>
@@ -206,21 +211,52 @@ const uploadsChunkEndpoint = API.v1.post(
- await Uploads.updateOne({ _id: uploadId }, { $addToSet: { 'resumable.uploadedChunks': Number(chunkIndex) } });
+ // Validate cumulative size
+ const chunkFiles = await readdir(path.join(TEMP_UPLOAD_DIR, uploadId as string));
+ let cumulativeSize = 0;
+ for (const cf of chunkFiles) {
</file context>
| return API.v1.notFound(); | ||
| } | ||
|
|
||
| if (!(await canAccessRoomIdAsync(this.userId, session.rid))) { |
Contributor
There was a problem hiding this comment.
P2: Race condition: once /v1/uploads/complete is in flight, a user-triggered cancel can race against finalization. The cancel handler will send /v1/uploads/cancel and remove the local UI item, but the server-side complete endpoint doesn't check whether the session has been cancelled before merging chunks and inserting into FileUpload. This can leave an orphaned file in storage. Either disable the cancel button on the client once finalization begins, or have the /uploads/complete endpoint atomically check for cancellation (e.g., verify session.uploading === true) before writing the final file.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/uploads.ts, line 295:
<comment>Race condition: once `/v1/uploads/complete` is in flight, a user-triggered cancel can race against finalization. The cancel handler will send `/v1/uploads/cancel` and remove the local UI item, but the server-side complete endpoint doesn't check whether the session has been cancelled before merging chunks and inserting into `FileUpload`. This can leave an orphaned file in storage. Either disable the cancel button on the client once finalization begins, or have the `/uploads/complete` endpoint atomically check for cancellation (e.g., verify `session.uploading === true`) before writing the final file.</comment>
<file context>
@@ -256,6 +292,10 @@ const uploadsCompleteEndpoint = API.v1.post(
return API.v1.notFound();
}
+ if (!(await canAccessRoomIdAsync(this.userId, session.rid))) {
+ return API.v1.failure('Unauthorized');
+ }
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/client/lib/chats/uploads.ts (1)
231-234:⚠️ Potential issue | 🟠 MajorCancel is not authoritative once finalization starts.
After line 233 starts
/v1/uploads/complete, there's no mechanism to prevent cancellation. If the user cancels during finalization,cancelHandlerremoves the local upload and calls/v1/uploads/cancel, which races with the server-side finalization. This can result in orphaned uploads or inconsistent state.Consider tracking a
finalizingflag before calling complete and havingcancelHandlerskip server-side cancel if finalization has started.💡 Suggested approach
+ let finalizing = false; + const cancelHandler = async () => { cancelled = true; if (xhr) { xhr.abort(); } - if (uploadId) { + if (uploadId && !finalizing) { try { await sdk.rest.post('/v1/uploads/cancel', { uploadId }); } catch (e) { // ignore } } this.set(this.uploads.filter((upload) => upload.id !== id)); }; // ... later in the code ... if (cancelled) return; + finalizing = true; const { file: finalFile } = (await sdk.rest.post('/v1/uploads/complete', { uploadId })) as any;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/lib/chats/uploads.ts` around lines 231 - 234, The upload finalization call in the block that posts to '/v1/uploads/complete' isn't protected from concurrent cancellation; set a per-upload "finalizing" flag on the upload record (e.g., via this.updateUpload(id, { finalizing: true })) immediately before calling sdk.rest.post('/v1/uploads/complete', { uploadId }) and clear it after completion, and modify the cancelHandler to check the upload's finalizing flag and skip sending the server-side cancel request if finalizing is true (but still remove/mark the local upload appropriately). Ensure references include the upload id variable used in the snippet, the sdk.rest.post('/v1/uploads/complete', ...) call, the this.updateUpload(...) call, and the cancelHandler so reviewers can locate and verify the changes.apps/meteor/app/api/server/v1/uploads.ts (1)
33-44:⚠️ Potential issue | 🟠 MajorResumable upload state is node-local and unaware of instance routing.
Lines 181-182 create the session directory on the handling instance, line 244 writes chunks to that local filesystem, and lines 302-318 read from it during completion. The
Uploadsdocument does not track which instance created the session (instanceIdfield is absent), so load-balanced deployments without sticky sessions will route chunks to arbitrary instances, causing failures.Either:
- Add
instanceIdtracking and restore instance-aware request routing, or- Move temporary chunk storage to shared infrastructure (database GridFS or shared filesystem), or
- Explicitly document that resumable uploads require sticky sessions on the load balancer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 33 - 44, The resumable upload implementation relies on node-local TEMP_UPLOAD_DIR and ensureTempDir so chunks and session dirs are tied to whichever instance handled the request, but the Uploads document lacks an instanceId so load-balanced (non-sticky) routing will break; fix by either 1) adding an instanceId field to the Uploads session record when the session is created and use that to route subsequent chunk/complete requests to the same instance (update the session-creation code, the chunk-write handler, and the completion handler to read/write and respect Uploads.instanceId), or 2) replace local file storage under TEMP_UPLOAD_DIR with shared storage (e.g., GridFS or a network/shared filesystem) so functions that write chunks and read them on completion no longer assume local disk, or 3) if you cannot change storage/routing, document clearly that resumable uploads require sticky sessions; pick one approach and implement the corresponding changes in the session creation, chunk write, and completion code paths referenced by ensureTempDir/TEMP_UPLOAD_DIR and the Uploads document handling.
🧹 Nitpick comments (2)
apps/meteor/app/api/server/v1/uploads.ts (2)
15-15: Remove unused import.
promisifyis imported but never used in this file since all filesystem operations usefs.promisesdirectly.-import { promisify } from 'util';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` at line 15, Remove the unused import of promisify from the top of the file: delete the "promisify" import (the import line that currently includes promisify) because the module uses fs.promises for filesystem operations; ensure no other references to promisify remain in the file (e.g., check any helper functions) and run a quick build or lint to confirm the unused import warning is gone.
144-149: Consider adding proper type definitions for request parameters.The extensive use of
as anycasts (lines 145, 208, 284, 384) bypasses TypeScript safety. Define parameter schemas with proper types similar touploadsDeleteParamsSchemapattern used earlier in the file.💡 Example improvement
type UploadsInitParams = { rid: string; fileName: string; fileSize: number; fileType: string; chunkSize: number; }; const uploadsInitParamsSchema = { type: 'object', properties: { rid: { type: 'string' }, fileName: { type: 'string' }, fileSize: { type: 'number' }, fileType: { type: 'string' }, chunkSize: { type: 'number' }, }, required: ['rid', 'fileName', 'fileSize', 'fileType', 'chunkSize'], additionalProperties: false, }; const isUploadsInitParams = ajv.compile<UploadsInitParams>(uploadsInitParamsSchema);Then use
body: isUploadsInitParamsin the endpoint options.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/uploads.ts` around lines 144 - 149, Replace the unsafe "as any" casts used in the action handlers (e.g., the action() in uploads.ts and other handlers referencing bodyParams at lines where as any is used) with properly defined TypeScript types and AJV schemas: define types like UploadsInitParams { rid: string; fileName: string; fileSize: number; fileType: string; chunkSize: number }, create a corresponding uploadsInitParamsSchema (properties, required, additionalProperties: false), compile it with ajv.compile<UploadsInitParams>(...) to get isUploadsInitParams, and wire that validator into the endpoint options (body: isUploadsInitParams) so you can safely type this.bodyParams as UploadsInitParams inside the function; apply the same pattern for the other occurrences (the handlers referenced at the other as-any sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 209-226: The xhr variable is incorrectly typed as "{ abort: () =>
void } | undefined" while sdk.rest.upload() actually returns an XMLHttpRequest;
update the declaration of xhr to "XMLHttpRequest | undefined" so you can remove
the unnecessary cast when assigning currentXhr (used around the upload promise
and handlers) and let onload/onerror/onabort access XMLHttpRequest properties
directly.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 33-44: The resumable upload implementation relies on node-local
TEMP_UPLOAD_DIR and ensureTempDir so chunks and session dirs are tied to
whichever instance handled the request, but the Uploads document lacks an
instanceId so load-balanced (non-sticky) routing will break; fix by either 1)
adding an instanceId field to the Uploads session record when the session is
created and use that to route subsequent chunk/complete requests to the same
instance (update the session-creation code, the chunk-write handler, and the
completion handler to read/write and respect Uploads.instanceId), or 2) replace
local file storage under TEMP_UPLOAD_DIR with shared storage (e.g., GridFS or a
network/shared filesystem) so functions that write chunks and read them on
completion no longer assume local disk, or 3) if you cannot change
storage/routing, document clearly that resumable uploads require sticky
sessions; pick one approach and implement the corresponding changes in the
session creation, chunk write, and completion code paths referenced by
ensureTempDir/TEMP_UPLOAD_DIR and the Uploads document handling.
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 231-234: The upload finalization call in the block that posts to
'/v1/uploads/complete' isn't protected from concurrent cancellation; set a
per-upload "finalizing" flag on the upload record (e.g., via
this.updateUpload(id, { finalizing: true })) immediately before calling
sdk.rest.post('/v1/uploads/complete', { uploadId }) and clear it after
completion, and modify the cancelHandler to check the upload's finalizing flag
and skip sending the server-side cancel request if finalizing is true (but still
remove/mark the local upload appropriately). Ensure references include the
upload id variable used in the snippet, the
sdk.rest.post('/v1/uploads/complete', ...) call, the this.updateUpload(...)
call, and the cancelHandler so reviewers can locate and verify the changes.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Line 15: Remove the unused import of promisify from the top of the file:
delete the "promisify" import (the import line that currently includes
promisify) because the module uses fs.promises for filesystem operations; ensure
no other references to promisify remain in the file (e.g., check any helper
functions) and run a quick build or lint to confirm the unused import warning is
gone.
- Around line 144-149: Replace the unsafe "as any" casts used in the action
handlers (e.g., the action() in uploads.ts and other handlers referencing
bodyParams at lines where as any is used) with properly defined TypeScript types
and AJV schemas: define types like UploadsInitParams { rid: string; fileName:
string; fileSize: number; fileType: string; chunkSize: number }, create a
corresponding uploadsInitParamsSchema (properties, required,
additionalProperties: false), compile it with
ajv.compile<UploadsInitParams>(...) to get isUploadsInitParams, and wire that
validator into the endpoint options (body: isUploadsInitParams) so you can
safely type this.bodyParams as UploadsInitParams inside the function; apply the
same pattern for the other occurrences (the handlers referenced at the other
as-any sites).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 204bb923-3d5d-4b9d-8570-a19dfd47e3ef
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
🧠 Learnings (16)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:50.283Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-12T17:12:42.624Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:42.624Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/uploads.tsapps/meteor/client/lib/chats/uploads.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/chats/uploads.ts
🔇 Additional comments (7)
apps/meteor/app/api/server/v1/uploads.ts (2)
309-330: LGTM!The stream write implementation properly handles backpressure with
drainevents and includes error handling withwriteStream.destroy()on failure.
366-408: LGTM!The cancel endpoint correctly validates session ownership and ensures only resumable sessions can be cancelled (line 391), addressing the previous concern about guarding against regular upload ids.
apps/meteor/client/lib/chats/uploads.ts (5)
52-60: LGTM!The pause and resume methods correctly update the upload status and emit the corresponding events. The implementation aligns with the UploadsAPI interface contract.
151-151: LGTM!The cancel handler is correctly registered before the first
await(line 166), ensuring cancellations during the init phase are properly handled.
174-182: LGTM!The
uploadIdis now captured immediately after init response, and server-side cleanup is properly attempted before early return when cancelled.
184-203: LGTM!The pause/resume implementation correctly waits for either a
resumingorcancellingevent, properly cleans up event listeners, and integrates well with the upload loop.
235-241: LGTM!Error handling correctly checks
!cancelledbefore setting error status, preventing cancelled uploads from briefly showing as failed. The finally block properly cleans up the cancel handler.
Comment on lines
+209
to
226
| await new Promise((resolve, reject) => { | ||
| xhr = sdk.rest.upload(`/v1/uploads/chunk?uploadId=${uploadId}&chunkIndex=${i}`, { | ||
| chunkData: chunk, | ||
| }); | ||
|
|
||
| this.updateUpload(id, { percentage: 0, error: new Error(i18n.t('FileUpload_Error')) }); | ||
| } catch (error) { | ||
| this.updateUpload(id, { percentage: 0, error: new Error(getErrorMessage(error)) }); | ||
| } | ||
| }; | ||
| const currentXhr = xhr as XMLHttpRequest; | ||
|
|
||
| this.once(`cancelling-${id}`, () => { | ||
| xhr.abort(); | ||
| this.set(this.uploads.filter((upload) => upload.id !== id)); | ||
| reject(new Error(i18n.t('FileUpload_Canceled'))); | ||
| currentXhr.onload = () => { | ||
| if (currentXhr.status === 200) { | ||
| resolve(currentXhr.response); | ||
| } else { | ||
| reject(new Error(currentXhr.statusText)); | ||
| } | ||
| }; | ||
|
|
||
| currentXhr.onerror = () => reject(new Error('Network error')); | ||
| currentXhr.onabort = () => reject(new Error('cancelled')); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for sdk.rest.upload implementation to verify return type
ast-grep --pattern 'upload($_, $_) {
$$$
}'
# Also check the SDKClient definition
rg -n "upload\s*[:(]" apps/meteor/app/utils/client/lib/SDKClient.ts -A 10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Find the SDKClient file and examine it
find apps/meteor -name "SDKClient*" -type f
# Also search for where sdk is instantiated and how rest.upload is defined
rg -l "sdk\.rest\.upload" apps/meteor/client/lib/chats/uploads.ts
# Look at the uploads.ts file around line 209-226
sed -n '190,240p' apps/meteor/client/lib/chats/uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 1790
🏁 Script executed:
# Read the SDKClient.ts file to find the upload method definition
cat -n apps/meteor/app/utils/client/lib/SDKClient.ts | head -100
# Search for the upload method specifically
rg -n "upload\s*\(" apps/meteor/app/utils/client/lib/SDKClient.ts -A 20Repository: RocketChat/Rocket.Chat
Length of output: 3584
🏁 Script executed:
# Search for where rest is assigned or defined in SDKClient
rg -n "rest\s*=" apps/meteor/app/utils/client/lib/SDKClient.ts -B 2 -A 5
# Look for the upload method in RestApiClient
find apps/meteor -name "RestApiClient*" -type f
# Search for rest.upload across the codebase to see if there are type hints
rg -n "rest\.upload" apps/meteor/client/lib/chats/uploads.ts -B 5 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 409
🏁 Script executed:
# Read RestApiClient.ts to find upload method
cat -n apps/meteor/app/utils/client/lib/RestApiClient.ts
# If file is very large, search for upload method specifically
wc -l apps/meteor/app/utils/client/lib/RestApiClient.tsRepository: RocketChat/Rocket.Chat
Length of output: 1884
🏁 Script executed:
# Find `@rocket.chat/api-client` package location
find . -path "*/node_modules" -prune -o -name "api-client" -type d -print
# Look for it in packages
find packages -name "*api-client*" -type d
# Check the package.json to find where api-client comes from
rg -n "@rocket.chat/api-client" apps/meteor/app/utils/client/lib/RestApiClient.ts
# Search for RestClient definition
rg -n "class RestClient" --type ts --type tsx
# Look in packages directory for the definition
fd -t f "RestClient" packages/ --extension ts --extension tsxRepository: RocketChat/Rocket.Chat
Length of output: 255
🏁 Script executed:
# Find upload method in RestClient
rg -n "upload\s*\(" packages/api-client/src/ -A 10
# Also look at RestClientInterface
cat -n packages/api-client/src/RestClientInterface.ts | head -50
# Check the main RestClient implementation file
find packages/api-client/src -name "*.ts" -type f | head -10Repository: RocketChat/Rocket.Chat
Length of output: 2733
🏁 Script executed:
# Search for upload method in RestClientInterface
rg -n "upload" packages/api-client/src/RestClientInterface.ts -A 10
# Also search in the entire api-client package
rg -n "upload" packages/api-client/src/ -A 5
# Check RestClient implementation
find packages/api-client/src -name "*.ts" -exec grep -l "class RestClient" {} \;Repository: RocketChat/Rocket.Chat
Length of output: 2347
🏁 Script executed:
# Find xhr variable declaration in uploads.ts
rg -n "xhr\s*=" apps/meteor/client/lib/chats/uploads.ts -B 5
# Look at the full context around the xhr variable declaration
sed -n '1,220p' apps/meteor/client/lib/chats/uploads.ts | grep -n "xhr" -B 3 -A 3Repository: RocketChat/Rocket.Chat
Length of output: 1062
Correct the xhr type annotation to match sdk.rest.upload return type.
Line 133 declares xhr as { abort: () => void } | undefined, but sdk.rest.upload() returns XMLHttpRequest. The narrow type forces an unnecessary cast at line 214. Change the type annotation to XMLHttpRequest | undefined to eliminate the cast and align with the actual return type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/client/lib/chats/uploads.ts` around lines 209 - 226, The xhr
variable is incorrectly typed as "{ abort: () => void } | undefined" while
sdk.rest.upload() actually returns an XMLHttpRequest; update the declaration of
xhr to "XMLHttpRequest | undefined" so you can remove the unnecessary cast when
assigning currentXhr (used around the upload promise and handlers) and let
onload/onerror/onabort access XMLHttpRequest properties directly.
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.
New Endpoints: Added
init
, chunk, complete, and
cancel
endpoints to handle the lifecycle of a resumable upload.
Persistent Chunks: Implemented temporary storage for file chunks using fs.promises. Chunks are stored in a session-specific directory and merged only when the upload is complete.
Cleanup Logic: Automatic cleanup of temporary files upon completion or cancellation.
2. Client-Side Logic
Chunked Upload Loop: Refactored the
UploadsStore
to slice files into 1MB chunks and upload them sequentially.
State Management: Added
pause
,
resume
, and
cancel▶️ ) buttons next to the upload progress bar.
methods to the store. The uploader accurately tracks progress based on the number of successful chunks.
Pause/Resume Persistence: The client waits for the "resuming" event before continuing the chunked loop, allowing for seamless network interruption handling.
3. UI Enhancements
Interactive Controls: Added Pause (⏸️) and Resume (
Real-time Feedback: The UI updates immediately when an upload is paused or resumed, providing clear feedback to the user.
Summary by CodeRabbit
New Features
Documentation