feat: Send multiple files into a single message #32703
Conversation
🦋 Changeset detectedLatest commit: 52dc433 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 |
dougfabris
force-pushed
the
feat/multiple_files_in_one_msg
branch
from
3e314a9 to
c93448c
Compare
coderabbitai
bot
added
stat: QA assured
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx (1)
12-20:⚠️ Potential issue | 🟡 MinorRemove stale props from
defaultProps.The
FileUploadModalcomponent accepts onlyonClose,onSubmit,file, andfileName(lines 29-34 of FileUploadModal.tsx). The test'sdefaultProps(lines 12-20) includes three properties that don't exist in the component's type definition:fileDescription,invalidContentType, andshowDescription. These are never used in the test and should be removed.apps/meteor/tests/e2e/page-objects/fragments/modals/file-upload-modal.ts (1)
14-16:⚠️ Potential issue | 🟡 MinorDead code:
fileDescriptionInputandsetDescriptionreference a removed UI element.The
FileUploadModalcomponent no longer renders a description field (removed inFileUploadModal.tsx), so these accessors will fail at runtime if ever called. They should be removed to keep the page object in sync with the UI.Proposed fix
- private get fileDescriptionInput() { - return this.root.getByRole('textbox', { name: 'File description' }); - } - private get updateButton() { return this.root.getByRole('button', { name: 'Update' }); } @@ -22,10 +18,6 @@ setName(fileName: string) { return this.fileNameInput.fill(fileName); } - - setDescription(description: string) { - return this.fileDescriptionInput.fill(description); - }Also applies to: 26-28
apps/meteor/client/views/room/body/hooks/useFileUploadDropTarget.ts (1)
50-61:⚠️ Potential issue | 🟠 MajorFile deduplication by size alone will silently drop distinct files that happen to share the same byte count.
Using only
file.sizeas the dedup key means any two files with identical sizes (but different names/content) will be collapsed to one. Consider using a composite key such as${file.name}-${file.size}-${file.lastModified}.Proposed fix
const getUniqueFiles = () => { const uniqueFiles: File[] = []; - const st: Set<number> = new Set(); + const st: Set<string> = new Set(); files.forEach((file) => { - const key = file.size; + const key = `${file.name}-${file.size}-${file.lastModified}`; if (!st.has(key)) { uniqueFiles.push(file); st.add(key); } }); return uniqueFiles; };
🤖 Fix all issues with AI agents
In `@apps/meteor/app/file-upload/server/methods/sendFileMessage.ts`:
- Around line 26-37: The validateFilesRequiredFields function currently only
checks key presence; update it to assert each required field has a meaningful
value by iterating requiredFields and verifying for each file that file[field]
is not null or undefined, that string fields ('_id','name','type') are non-empty
(trimmed length > 0), and that 'size' is a valid number (typeof === 'number' and
!isNaN(file.size)); throw the same Meteor.Error('error-invalid-file', 'Invalid
file') when any check fails so the assert still narrows to MinimalUploadData[].
- Line 115: The thumbnail URL is being built with the original file's name;
update the thumbUrl construction in sendFileMessage (where thumbUrl is assigned)
to use the thumbnail object's name property (thumbnail.name) instead of
file.name, e.g., call FileUpload.getPath with
`${thumbnail._id}/${encodeURI(thumbnail.name || '')}` so it points to the stored
thumb-{...} filename used in FileUpload.ts.
In `@apps/meteor/app/lib/server/functions/sendMessage.ts`:
- Around line 261-272: The code currently calls
Uploads.findByIds(uploadIdsToConfirm) and passes results to
parseMultipleFilesIntoMessageAttachments without ensuring ownership; before
calling parseMultipleFilesIntoMessageAttachments (and before setting
message.files/attachments/file) validate that every returned upload.userId ===
user._id and throw or abort on mismatch to prevent attaching foreign files.
Specifically, after Uploads.findByIds(...).toArray() check
uploadsToConfirm.some(u => u.userId !== user._id) and handle as unauthorized,
ensuring both Uploads.findByIds and parseMultipleFilesIntoMessageAttachments
(and any subsequent updateFileComplete usage) only operate on uploads owned by
user._id.
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts`:
- Around line 16-27: The getHeightAndWidthFromDataUrl function currently never
rejects if the Image fails to load, causing callers like getAttachmentForFile to
hang; add an img.onerror handler that rejects the returned Promise with a
meaningful Error (e.g., "Failed to load image" plus any event/message) and also
consider adding a timeout fallback to reject if load takes too long; update
getHeightAndWidthFromDataUrl to attach both img.onload (resolve) and img.onerror
(reject) so upstream upload/message flows can handle failures.
- Around line 180-189: The composedMessage currently forces t: 'e2e' and msg: ''
whenever e2eRoom is truthy, which can break non-converted messages; update the
composition logic for composedMessage so the E2E-specific fields (t: 'e2e' and
msg: '') are only added when both e2eRoom and shouldConvertSentMessages are true
(i.e., gate the E2E branch on shouldConvertSentMessages), and leave msg/content
untouched for cases where shouldConvertSentMessages is false so slash commands
and other non-encrypted messages keep their original msg.
- Around line 59-61: The current call to getHeightAndWidthFromDataUrl uses
window.URL.createObjectURL(fileToUpload.file) but never revokes the created
object URL; modify the upload flow in processMessageUploads.ts so you assign the
result of window.URL.createObjectURL(...) to a const (e.g., blobUrl), pass
blobUrl into getHeightAndWidthFromDataUrl, and then immediately call
window.URL.revokeObjectURL(blobUrl) in a finally block (or after awaiting the
dimensions) to ensure the object URL is always revoked and avoid leaking memory.
- Around line 173-178: The code calls getEncryptedContent with the unfiltered
filesToUpload and casts it to EncryptedUpload[], risking runtime errors; instead
build the encrypted-only array (the same filtered list used to create
filesToConfirm — e.g., filter filesToUpload for entries with url/id and with
encryptedFile present) and pass that filtered encryptedUploads array to
getEncryptedContent (when shouldConvertSentMessages is true), keeping references
to getEncryptedContent, filesToUpload, filesToConfirm, EncryptedUpload, and
e2eRoom.shouldConvertSentMessages to locate the change.
- Around line 151-153: The final "return continueSendingMessage()" is
unreachable because the preceding branches already return for both
failedUploads.length === 0 and > 0; remove the dead statement at the end of
processMessageUploads (or restructure so there is a single return point) to
eliminate the unreachable code and avoid confusion—look for the failedUploads
checks and the continueSendingMessage() call inside the processMessageUploads
function and delete the redundant trailing return.
In `@apps/meteor/client/lib/chats/flows/uploadFiles.ts`:
- Around line 52-58: The early return when checking
settings.peek('E2E_Enable_Encrypt_Files') in uploadNextFile causes the whole
upload queue to stop; change the logic to dispatch the error toast for the
current file (use dispatchToastMessage with t('Encrypted_file_not_allowed')) and
then continue processing the remaining items instead of returning — i.e., remove
the blocking return in uploadNextFile, mark/skip the current queue item as
failed or unencrypted as appropriate, and call the next recursion/continuation
(the same path that uploadNextFile uses to proceed to the next queue item) so
remaining files are not silently dropped.
- Around line 65-88: encryptFile can return falsy and currently the code simply
skips the upload block, leaving the queue stalled; update the handler around
e2eRoom.encryptFile(file) so that when encryptedFile is falsy you (1) call
uploadNextFile() to continue processing the queue and (2) surface an error to
the user (e.g., via an existing notification/error handler) so the failure isn’t
silent; keep uploadFile(encryptedFile.file, ...) only inside the truthy branch
and add an else branch that logs the failure and calls uploadNextFile().
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 162-198: The XHR onload handler (xhr.onload) is calling
JSON.parse(xhr.responseText) twice without guarding against parse errors; wrap
each JSON.parse call in a try/catch inside the xhr.onload so a malformed
response won't throw asynchronously. For the 400 branch (where error =
JSON.parse(...)) catch parse errors and set the corresponding upload's error
(via this.set on this.uploads.map matching id) to a descriptive Error (or
include xhr.responseText) so the upload record is updated instead of throwing;
do the same for the 200 branch (result = JSON.parse(...)) and on parse failure
set the upload.error and return early. Ensure you still handle the original
status checks (xhr.status === 400 / 200) and use the same identifiers
(xhr.onload, id, this.uploads, this.set) to locate and update the code.
- Around line 98-111: The Promise executor currently calls reject() for empty
files, oversized files, and invalidContentType but does not return, so execution
continues and later calls sdk.rest.upload(...) causing invalid uploads; update
the Promise block in the uploads flow so each reject() is immediately followed
by a return to exit the executor (i.e., after the empty-file check, the
maxFileSize check, and the invalidContentType check), ensuring that when the
Promise is rejected no further code (including the sdk.rest.upload call) runs.
In `@apps/meteor/client/views/room/composer/messageBox/MessageComposerFile.tsx`:
- Around line 60-73: The onKeyDown handler on MessageComposerFileComponent
(where you call handleOpenFilePreview) doesn't prevent the browser's default
action for the Space key, causing page scroll; update the onKeyDown to call
e.preventDefault() for the Space key (and optionally Enter) before invoking
handleOpenFilePreview so the interactive element activated by keyboard doesn't
trigger unintended scrolling or default form behaviors; locate the onKeyDown
prop in MessageComposerFileComponent and add the preventDefault call referencing
the same handler (handleOpenFilePreview) and using the event variable (e).
In
`@apps/meteor/client/views/room/composer/messageBox/MessageComposerFileComponent.tsx`:
- Around line 30-46: The Box element in MessageComposerFileComponent.tsx
currently has role='button' and tabIndex={0} but relies on spread props for
keyboard handlers, so in error states (MessageComposerFileError) it isn't
keyboard-activatable; add an internal onKeyDown handler on the Box that listens
for Enter and Space, prevents default for Space, and invokes the same activation
behavior as the mouse onClick (call props.onClick or trigger the component's
activation logic), and ensure this handler coexists with any onKeyDown passed
via {...props} (call props.onKeyDown if present). Reference the Box element,
role='button', tabIndex, previewWrapperStyle and the component
MessageComposerFileComponent when applying the change.
- Around line 20-27: The previewWrapperStyle CSS sets background-color to the
literal string 'surface-tint' which is invalid; change that line to interpolate
the resolved palette token using Palette.surface['surface-tint'] (i.e. replace
background-color: 'surface-tint'; with background-color:
${Palette.surface['surface-tint']};) so the style uses the actual color value
(previewWrapperStyle, Palette.surface and error are the relevant symbols to
locate).
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts`:
- Around line 107-112: The test step description is misleading: the code sets
FileUpload_MediaTypeWhiteList to "text/plain" but the test.step message says
"blacklisted setting set"; update the test.step description string in the
e2ee-file-encryption.spec.ts (the test wrapped by test.step and using
poHomeChannel methods like content.dragAndDropTxtFile, composer.getFileByName,
composer.removeFileByName) to reference the whitelist restriction (e.g.,
"whitelist set to text/plain, encrypted file upload should fail") so it
accurately reflects that a whitelist (not a blacklist) is being applied.
In `@apps/meteor/tests/e2e/file-upload.spec.ts`:
- Line 52: Replace no-op expect calls that pass a locator wrapper (e.g.,
poHomeChannel.composer.getFileByName(updatedFileName)) with real Playwright
assertions: await
expect(poHomeChannel.composer.getFileByName(updatedFileName)).toBeVisible() (or
another appropriate matcher like toHaveText/toBeAttached) for each occurrence
(the ones at the current diff and the other occurrences referenced around lines
63, 64, 118, 119) so the test actually verifies file presence/visibility.
- Around line 144-153: The test "should respect the maximum number of files
allowed per message: 10" is asserting the wrong filename; replace the current
assertion that checks for 'number6.png' with a check that the actual
drag-and-dropped file (the txt file dropped by
poHomeChannel.content.dragAndDropTxtFile) was rejected — i.e., capture or use
the txt filename (e.g., 'any_file.txt' or the value returned/used by
dragAndDropTxtFile) and assert
poHomeChannel.composer.getFileByName(<txt-file-name>) is not visible while still
asserting getFilesInComposer() has count 10; update the reference in the
assertion accordingly.
In `@apps/meteor/tests/e2e/fixtures/responses/mediaResponse.ts`:
- Around line 5-13: The current try/catch in waitForMediaResponse is ineffective
because page.waitForResponse returns a Promise and won't throw synchronously;
remove the try/catch and simply return the promise from
page.waitForResponse((response) => isMediaResponse(response)) directly in the
waitForMediaResponse function so errors/timeouts surface to the caller when they
await the returned promise.
In `@packages/model-typings/src/models/IBaseUploadsModel.ts`:
- Around line 13-14: The interface method confirmTemporaryFiles currently allows
a return of undefined but the BaseUploadModel.confirmTemporaryFiles
implementation always returns this.updateMany(...); fix by making the signature
and implementation consistent: either remove the "| undefined" from
confirmTemporaryFiles in IBaseUploadsModel to return Promise<Document |
UpdateResult> (preferred if you want no early-exit), or add an empty-array guard
to BaseUploadModel.confirmTemporaryFiles that returns undefined when
fileIds.length === 0 to match the single-file confirmTemporaryFile behavior;
update the signature or implementation accordingly so the declared return type
matches the actual return value.
🧹 Nitpick comments (24)
apps/meteor/client/providers/ImageGalleryProvider.tsx (1)
26-36: Remove inline comment block per coding guidelines; the URL-parsing workaround is fragile.
The comment block (Lines 26–32) violates the project guideline to avoid code comments in implementation files. Consider moving this context to a linked issue or commit message instead.
Parsing
data-srcby splitting on'/file-upload/'is brittle — any change to the URL structure will silently fail. The comment itself acknowledges the proper fix is to store the file ID in a data attribute on each attachment element. Consider tracking this as a follow-up issue.As per coding guidelines:
**/*.{ts,tsx,js}: "Avoid code comments in the implementation".apps/meteor/client/views/room/composer/messageBox/MessageComposerFileLoader.tsx (2)
5-5: Remove the TODO comment from the implementation.As per coding guidelines, code comments should be avoided in the implementation. Consider tracking this in an issue instead.
7-19: Consider moving the CSS definition outside the component.
customCSSis recomputed on every render. Whilecssmay cache internally, hoisting it to module scope avoids the repeated call entirely and makes the intent clearer.♻️ Suggested refactor
+const spinAnimation = css` + animation: spin-animation 0.8s linear infinite; + + `@keyframes` spin-animation { + from { + transform: rotate(0deg); + } + + to { + transform: rotate(360deg); + } + } +`; + const MessageComposerFileLoader = (props: ComponentProps<typeof Box>) => { - const customCSS = css` - animation: spin-animation 0.8s linear infinite; - - `@keyframes` spin-animation { - from { - transform: rotate(0deg); - } - - to { - transform: rotate(360deg); - } - } - `; - return ( - <Box className={customCSS} is='svg' ...> + <Box className={spinAnimation} is='svg' ...>packages/models/src/models/BaseUploadModel.ts (1)
91-106: Consider guarding against an emptyfileIdsarray.If
fileIdsis empty,$in: []matches nothing and theupdateManybecomes a wasted round-trip. An early return would be consistent with the guard pattern inupdateFileComplete/confirmTemporaryFile.♻️ Suggested guard
confirmTemporaryFiles(fileIds: string[], userId: string): Promise<Document | UpdateResult> | undefined { + if (!fileIds.length) { + return; + } + const filter = {apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerModal.tsx (1)
128-151: MissingawaitonuploadFile(file)— error/loading state may misbehave.Line 147 calls the async
uploadFilewithoutawait. Because it's inside the outertryblock, the outer function will return (and the calling code will see the promise as resolved) beforeuploadFilefinishes. WhileuploadFilehas its owntry/catch/finally, any unhandled rejection fromonUploadwill become an unhandled promise rejection rather than being caught.This appears pre-existing, but now is a good time to fix it since you're already touching this line.
Proposed fix
- uploadFile(file); + await uploadFile(file);apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (1)
174-184: Duplicate locators — these are already defined inFileUploadModalpage object.
inputFileUploadName,btnUpdateFileUpload, andbtnCancelUpdateFileUploadduplicate the locators inFileUploadModal(seefile-upload-modal.tslines 10-20, 30-33). Per the Page Object Model pattern, modal interactions should be delegated to theFileUploadModalclass rather than re-declared here.If
FileUploadModalisn't usable in certain contexts, consider exposing the needed methods there rather than duplicating locators.apps/meteor/client/views/room/contextualBar/Threads/components/ThreadChat.tsx (1)
56-62: Optional chaining onchatis redundant after the null guard.Lines 57 and 61 use
chat?.messageEditingbut the throw at line 26 guaranteeschatis non-null. The optional chaining is harmless but inconsistent.♻️ Suggested cleanup
const handleNavigateToPreviousMessage = useCallback((): void => { - chat?.messageEditing.toPreviousMessage(); + chat.messageEditing.toPreviousMessage(); - }, [chat?.messageEditing]); + }, [chat.messageEditing]); const handleNavigateToNextMessage = useCallback((): void => { - chat?.messageEditing.toNextMessage(); + chat.messageEditing.toNextMessage(); - }, [chat?.messageEditing]); + }, [chat.messageEditing]);apps/meteor/tests/end-to-end/api/methods.ts (1)
2092-2115: Good test coverage for the max files limit.The test correctly validates the boundary condition (11 > 10 files) and asserts both the status code and the specific error identifier. Consider also adding a complementary test for the boundary (exactly 10 files) to verify the limit is inclusive.
apps/meteor/tests/end-to-end/api/moderation.ts (1)
810-811: Fragile parsing —JSON.parseonsendMessageResponse.body.messagewithout a guard.If the
sendMessagemethod call fails or returns an unexpected shape,JSON.parsewill throw a confusing error. Consider adding an assertion on the response structure before parsing, e.g., checking thatsendMessageResponse.body.messageis a string.apps/meteor/tests/e2e/image-upload.spec.ts (1)
37-41: Verify thereadonlyattribute assertion captures the intended error state.The test name says "should show error indicator when upload fails," but the assertion only checks for a
readonlyattribute. Consider also asserting that an error indicator (visual cue, aria attribute, or error text) is visible to ensure the test validates the error UX, not just a side-effect attribute.apps/meteor/client/views/room/composer/messageBox/MessageComposerFileError.tsx (1)
6-10: Consider renaming the local type to avoid confusion withMessageComposerFileComponent's props type.Both this file and
MessageComposerFileComponent.tsxdefine a type namedMessageComposerFileComponentPropswith different shapes. While they're file-scoped, this could cause confusion during maintenance. Something likeMessageComposerFileErrorPropswould be clearer.apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
384-384: Confusing variable name — consider inverting the boolean.
shouldDisableDueUploadsistrueboth when there are no uploads (normal empty state) and when uploads are in progress. The name suggests the disable reason is "uploads" but the primary case is actually "nothing to send." An inverted, positive name would be clearer and simplify the downstream expressions.♻️ Suggested refactor
- const shouldDisableDueUploads = !hasUploads || isUploading; + const canSendUploads = hasUploads && !isUploading;Then update the usages:
- disabled={!canSend || (!typing && !isEditing && shouldDisableDueUploads)} + disabled={!canSend || (!typing && !isEditing && !canSendUploads)} - secondary={typing || isEditing || !shouldDisableDueUploads} + secondary={typing || isEditing || canSendUploads} - info={typing || isEditing || !shouldDisableDueUploads} + info={typing || isEditing || canSendUploads}apps/meteor/client/lib/chats/flows/sendMessage.ts (1)
84-85: Remove code comments per project guidelines.As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation.♻️ Suggested fix
- // When editing an encrypted message with files, preserve the original attachments/files - // This ensures they're included in the re-encryption process if (mid) {apps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useFileUploadAction.ts (1)
38-40: Stale ref in cleanup — capturefileInputRef.currentbefore setup.
fileInputRef.currentmay differ between the effect body (line 38) and the cleanup (line 39) if the ref changes between renders. Capture the node once to ensure the same element is used for bothaddEventListenerandremoveEventListener.♻️ Suggested fix
useEffect(() => { + const fileInput = fileInputRef?.current; const resetFileInput = () => { - if (!fileInputRef?.current) { + if (!fileInput) { return; } - fileInputRef.current.value = ''; + fileInput.value = ''; }; const handleUploadChange = async () => { const { getMimeType } = await import('../../../../../../../app/utils/lib/mimeTypes'); - const filesToUpload = Array.from(fileInputRef?.current?.files ?? []).map((file) => { + const filesToUpload = Array.from(fileInput?.files ?? []).map((file) => { Object.defineProperty(file, 'type', { value: getMimeType(file.type, file.name), }); return file; }); chat?.flows.uploadFiles({ files: filesToUpload, uploadsStore, resetFileInput }); }; - fileInputRef.current?.addEventListener('change', handleUploadChange); - return () => fileInputRef?.current?.removeEventListener('change', handleUploadChange); + fileInput?.addEventListener('change', handleUploadChange); + return () => fileInput?.removeEventListener('change', handleUploadChange); }, [chat, fileInputRef, uploadsStore]);apps/meteor/client/lib/chats/Upload.ts (1)
3-9: Consider makingEncryptedFilefieldsreadonlyfor consistency.
NonEncryptedUploadfields are allreadonly, butEncryptedFilefields (lines 18-24) are mutable. If these aren't mutated after creation, addingreadonlywould align with the immutable pattern used elsewhere in this type hierarchy.Also applies to: 11-14, 18-24
apps/meteor/client/lib/chats/flows/processMessageUploads.ts (1)
92-99:settings.peekreturnsSettingValue, cast tobooleanis unsafe.Line 99 casts the return value to
boolean. If the setting is undefined or missing, this will silently becomeundefinedtreated asboolean. Consider using a safer default.Proposed fix
- const multiFilePerMessageEnabled = settings.peek('FileUpload_EnableMultipleFilesPerMessage') as boolean; + const multiFilePerMessageEnabled = Boolean(settings.peek('FileUpload_EnableMultipleFilesPerMessage'));apps/meteor/client/lib/chats/flows/uploadFiles.ts (1)
23-31:uploadsStore.send()is fire-and-forget — errors are silently swallowed.
sendreturns aPromise<void>but is called withoutawaitand without a.catch(). If the upload initiation fails (e.g., network error, storage issue), the error is lost and the user gets no feedback.Proposed fix: handle errors from send
- const uploadFile = (file: File, encrypted?: EncryptedFileUploadContent) => { + const uploadFile = async (file: File, encrypted?: EncryptedFileUploadContent) => { + try { if (encrypted) { - uploadsStore.send(file, encrypted); + await uploadsStore.send(file, encrypted); } else { - uploadsStore.send(file); + await uploadsStore.send(file); } - - uploadNextFile(); + } catch (error) { + dispatchToastMessage({ type: 'error', message: error }); + } finally { + uploadNextFile(); + } };apps/meteor/app/lib/server/functions/sendMessage.ts (1)
255-259:updateFileNamesside-effects execute before message validation.
updateFileNamesmodifies DB records (lines 228, 231) beforevalidateMessageis called on line 274. If validation fails, file names will have been updated for uploads that were never sent. Since these are temporary uploads that would eventually expire, the impact is low — but it breaks the principle of validating before mutating.Consider moving updateFileNames after validation
+ await validateMessage(message, room, user); + prepareMessageObject(message, room._id, user); + if (filesToConfirm) { await updateFileNames(filesToConfirm, isE2E); } // ... rest of file processing ... - - await validateMessage(message, room, user); - prepareMessageObject(message, room._id, user);apps/meteor/client/lib/chats/ChatAPI.ts (1)
103-107: Redundant| undefinedon optional property.On line 105, the
?already makes the property optional (allowingundefined), so| undefinedis redundant.Proposed fix
export type EncryptedFileUploadContent = { rawFile: File; - fileContent: { raw: Partial<IUpload>; encrypted?: IE2EEMessage['content'] | undefined }; + fileContent: { raw: Partial<IUpload>; encrypted?: IE2EEMessage['content'] }; encryptedFile: EncryptedFile; };apps/meteor/client/lib/chats/uploads.ts (5)
46-78:editUploadFileNameisasyncbut neverawaits — return type mismatch withUploadsAPI.The method is declared
async(returningPromise<void>) but performs no asynchronous operations. TheUploadsAPItype declareseditUploadFileNameas returningvoid. While TypeScript allowsPromise<void>to be assigned tovoid, the unnecessaryasyncis misleading. If the intent is to eventually add async operations (e.g., server-side rename), consider keeping it; otherwise, dropasync.Proposed fix
- editUploadFileName = async (uploadId: Upload['id'], fileName: Upload['file']['name']): Promise<void> => { + editUploadFileName = (uploadId: Upload['id'], fileName: Upload['file']['name']): void => {
130-141: Repeated upload-update-by-id pattern is a candidate for a helper.The pattern
this.set(this.uploads.map((upload) => upload.id !== id ? upload : { ...upload, ...changes }))appears at least five times insendalone. Extracting it would reduce duplication and cognitive load.Proposed helper
+ private updateUpload(id: Upload['id'], patch: Partial<Upload>): void { + this.set(this.uploads.map((upload) => (upload.id !== id ? upload : { ...upload, ...patch }))); + }Then call sites become:
this.updateUpload(id, { percentage: Math.round(progress) || 0 });Based on learnings: "Follow DRY (Do not Repeat Yourself) principles by extracting reusable logic into helper functions".
Also applies to: 144-156, 166-177, 183-195, 207-219
104-104: Remove code comment per coding guidelines.Line 104 has a comment
// -1 maxFileSize means there is no limit. As per coding guidelines for**/*.{ts,tsx,js}files, code comments in the implementation should be avoided.
14-23:uploadsfield is public — consider making itprivate.
uploadson line 23 is accessible on the class instance, but all external access is meant to go throughget(). SincecreateUploadsAPIreturnsUploadsAPI(which doesn't expose a rawuploadsproperty), this is not exploitable externally, but marking itprivatewould be more defensive and self-documenting.Proposed fix
- uploads: readonly Upload[] = []; + private uploads: readonly Upload[] = [];
82-96: Cancellation after the Promise resolves leaves a danglingoncelistener.On line 200, a
oncelistener is registered forcancelling-${id}. If the upload completes successfully (Promise resolves via theloadcallback), this listener is never removed. Whileonceprevents it from firing multiple times, it stays registered until fired or theUploadsStoreis GC'd. For long-lived store instances handling many uploads, these accumulate.Consider removing the listener when the promise settles (resolves or rejects from non-cancel paths).
Also applies to: 200-204
| function validateFilesRequiredFields(files: Partial<IUpload>[]): asserts files is MinimalUploadData[] { | ||
| const requiredFields = ['_id', 'name', 'type', 'size']; | ||
| requiredFields.forEach((field) => { | ||
| if (!Object.keys(file).includes(field)) { | ||
| throw new Meteor.Error('error-invalid-file', 'Invalid file'); | ||
| for (const file of files) { | ||
| const fields = Object.keys(file); | ||
|
|
||
| for (const field of requiredFields) { | ||
| if (!fields.includes(field)) { | ||
| throw new Meteor.Error('error-invalid-file', 'Invalid file'); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Validation checks key presence but not value truthiness.
Object.keys(file).includes(field) passes even when the value is null, undefined, or ''. A caller could send { _id: null, name: '', type: undefined, size: 0 } and this validation would not reject it, yet downstream code (e.g., Uploads.updateFileComplete(file._id, ...)) would break or produce corrupt data.
Consider checking that the value is actually present and meaningful:
🛡️ Proposed fix
function validateFilesRequiredFields(files: Partial<IUpload>[]): asserts files is MinimalUploadData[] {
const requiredFields = ['_id', 'name', 'type', 'size'];
for (const file of files) {
- const fields = Object.keys(file);
-
for (const field of requiredFields) {
- if (!fields.includes(field)) {
+ if (!(field in file) || file[field as keyof IUpload] == null) {
throw new Meteor.Error('error-invalid-file', 'Invalid file');
}
}
}
}
🤖 Prompt for AI Agents
In `@apps/meteor/app/file-upload/server/methods/sendFileMessage.ts` around lines
26 - 37, The validateFilesRequiredFields function currently only checks key
presence; update it to assert each required field has a meaningful value by
iterating requiredFields and verifying for each file that file[field] is not
null or undefined, that string fields ('_id','name','type') are non-empty
(trimmed length > 0), and that 'size' is a valid number (typeof === 'number' and
!isNaN(file.size)); throw the same Meteor.Error('error-invalid-file', 'Invalid
file') when any check fails so the assert still narrows to MinimalUploadData[].
| roomId, | ||
| uid, | ||
| ); | ||
| const thumbUrl = FileUpload.getPath(`${thumbnail._id}/${encodeURI(file.name || '')}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's look at the file and the specific area mentioned
head -n 130 apps/meteor/app/file-upload/server/methods/sendFileMessage.ts | tail -n 30Repository: RocketChat/Rocket.Chat
Length of output: 1065
🏁 Script executed:
#!/bin/bash
# Check the structure around the file upload logic to understand thumbnail object properties
rg -n "thumbnail\._id|thumbnail\.name" apps/meteor/app/file-upload/ --type=ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Search for how thumbnails are created/stored to understand naming conventions
rg -n "getThumbnail|storeThumbnail|thumbnail" apps/meteor/app/file-upload/server/ --type=ts | head -40Repository: RocketChat/Rocket.Chat
Length of output: 2274
🏁 Script executed:
#!/bin/bash
# Check how getPath is used with other files to understand naming conventions
rg -n "FileUpload.getPath" apps/meteor/app/file-upload/server/ --type=ts -B1 -A1 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1057
🏁 Script executed:
#!/bin/bash
# Look at the uploadImageThumbnail method to understand what name is assigned to the thumbnail
rg -n "uploadImageThumbnail" apps/meteor/app/file-upload/ --type=ts -A15 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2939
🏁 Script executed:
#!/bin/bash
# Check what thumbnailFileName variable is and how it's constructed
rg -n "thumbFileName|thumbFileType" apps/meteor/app/file-upload/ --type=ts -B2 -A2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 2588
Use thumbnail.name instead of file.name for thumbnail URL construction.
At line 115, the thumbnail URL is constructed using the original file's name (file.name) rather than the thumbnail's actual name (thumbnail.name). The thumbnail is stored with name thumb-${thumbFileName} (see FileUpload.ts:352), so the URL should reference that name, not the original file name.
🔧 Proposed fix
- const thumbUrl = FileUpload.getPath(`${thumbnail._id}/${encodeURI(file.name || '')}`);
+ const thumbUrl = FileUpload.getPath(`${thumbnail._id}/${encodeURI(thumbnail.name || '')}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const thumbUrl = FileUpload.getPath(`${thumbnail._id}/${encodeURI(file.name || '')}`); | |
| const thumbUrl = FileUpload.getPath(`${thumbnail._id}/${encodeURI(thumbnail.name || '')}`); |
🤖 Prompt for AI Agents
In `@apps/meteor/app/file-upload/server/methods/sendFileMessage.ts` at line 115,
The thumbnail URL is being built with the original file's name; update the
thumbUrl construction in sendFileMessage (where thumbUrl is assigned) to use the
thumbnail object's name property (thumbnail.name) instead of file.name, e.g.,
call FileUpload.getPath with `${thumbnail._id}/${encodeURI(thumbnail.name ||
'')}` so it points to the stored thumb-{...} filename used in FileUpload.ts.
| const uploadIdsToConfirm = filesToConfirm?.map(({ _id }) => _id); | ||
|
|
||
| if (uploadIdsToConfirm?.length && !isE2E) { | ||
| const uploadsToConfirm: Partial<IUpload>[] = await Uploads.findByIds(uploadIdsToConfirm).toArray(); | ||
| const { files, attachments } = await parseMultipleFilesIntoMessageAttachments(uploadsToConfirm, message.rid, user); | ||
| message.files = files; | ||
| message.attachments = attachments; | ||
| // For compatibility with older integrations, we save the first file to the `file` attribute of the message | ||
| if (files.length) { | ||
| message.file = files[0]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Uploads model/collection implementations
fd -e ts -e js | xargs grep -l "findByIds\|confirmTemporaryFiles" | grep -E "(upload|file)" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 502
🏁 Script executed:
#!/bin/bash
# Search for Uploads class/model definition
rg -n "class Uploads|export.*Uploads\s*=" --type=ts -A 3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1712
🏁 Script executed:
#!/bin/bash
# Find where Uploads methods are defined
fd -e ts -e js -path "*/uploads*" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Read the Uploads model
wc -l packages/models/src/models/Uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Find method implementations in Uploads model
cat packages/models/src/models/Uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 2888
🏁 Script executed:
#!/bin/bash
# Search for confirmTemporaryFiles implementation
rg -n "confirmTemporaryFiles" --type ts -A 20 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 3647
🏁 Script executed:
#!/bin/bash
# Search for parseMultipleFilesIntoMessageAttachments
rg -n "parseMultipleFilesIntoMessageAttachments" --type ts -B 2 -A 15 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7178
🏁 Script executed:
#!/bin/bash
# Find updateFileComplete implementation
rg -n "updateFileComplete" --type ts -B 2 -A 15 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 6181
🏁 Script executed:
#!/bin/bash
# Check what updateFileComplete does with userId
rg -n "updateFileComplete\s*\(" --type ts -B 2 -A 20 | head -200Repository: RocketChat/Rocket.Chat
Length of output: 7676
🏁 Script executed:
#!/bin/bash
# Look for the full BaseUploadModel to see all method implementations
cat packages/models/src/models/BaseUploadModel.tsRepository: RocketChat/Rocket.Chat
Length of output: 3487
Unauthorized file attachment via unvalidated file ID references — security vulnerability.
Uploads.findByIds(uploadIdsToConfirm) on line 264 fetches uploads by client-provided IDs without filtering by user._id. A malicious client can reference file IDs belonging to other users. While updateFileComplete (called inside parseMultipleFilesIntoMessageAttachments) includes a userId ownership check in its filter, the files are already processed and attached to the message before that update is attempted. If the update fails due to ownership mismatch, the foreign files remain attached to the message.
Validate file ownership before processing:
const uploadsToConfirm: Partial<IUpload>[] = await Uploads.findByIds(uploadIdsToConfirm).toArray();
// Validate all uploads belong to the current user
if (uploadsToConfirm.some(upload => upload.userId !== user._id)) {
throw new Error('Unauthorized');
}
const { files, attachments } = await parseMultipleFilesIntoMessageAttachments(uploadsToConfirm, message.rid, user);
🤖 Prompt for AI Agents
In `@apps/meteor/app/lib/server/functions/sendMessage.ts` around lines 261 - 272,
The code currently calls Uploads.findByIds(uploadIdsToConfirm) and passes
results to parseMultipleFilesIntoMessageAttachments without ensuring ownership;
before calling parseMultipleFilesIntoMessageAttachments (and before setting
message.files/attachments/file) validate that every returned upload.userId ===
user._id and throw or abort on mismatch to prevent attaching foreign files.
Specifically, after Uploads.findByIds(...).toArray() check
uploadsToConfirm.some(u => u.userId !== user._id) and handle as unauthorized,
ensuring both Uploads.findByIds and parseMultipleFilesIntoMessageAttachments
(and any subsequent updateFileComplete usage) only operate on uploads owned by
user._id.
| const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => { | ||
| return new Promise((resolve) => { | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| resolve({ | ||
| height: img.height, | ||
| width: img.width, | ||
| }); | ||
| }; | ||
| img.src = dataURL; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Promise never rejects on image load failure — upload flow will hang.
If the image is corrupt or fails to load, img.onerror is never handled, so the returned promise hangs indefinitely. This blocks the entire getAttachmentForFile call and consequently the message send flow.
🐛 Proposed fix: add an onerror handler
const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => {
- return new Promise((resolve) => {
+ return new Promise((resolve, reject) => {
const img = new Image();
img.onload = () => {
resolve({
height: img.height,
width: img.width,
});
};
+ img.onerror = () => {
+ reject(new Error('Failed to load image for dimensions'));
+ };
img.src = dataURL;
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => { | |
| return new Promise((resolve) => { | |
| const img = new Image(); | |
| img.onload = () => { | |
| resolve({ | |
| height: img.height, | |
| width: img.width, | |
| }); | |
| }; | |
| img.src = dataURL; | |
| }); | |
| }; | |
| const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => { | |
| return new Promise((resolve, reject) => { | |
| const img = new Image(); | |
| img.onload = () => { | |
| resolve({ | |
| height: img.height, | |
| width: img.width, | |
| }); | |
| }; | |
| img.onerror = () => { | |
| reject(new Error('Failed to load image for dimensions')); | |
| }; | |
| img.src = dataURL; | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts` around lines 16
- 27, The getHeightAndWidthFromDataUrl function currently never rejects if the
Image fails to load, causing callers like getAttachmentForFile to hang; add an
img.onerror handler that rejects the returned Promise with a meaningful Error
(e.g., "Failed to load image" plus any event/message) and also consider adding a
timeout fallback to reject if load takes too long; update
getHeightAndWidthFromDataUrl to attach both img.onload (resolve) and img.onerror
(reject) so upstream upload/message flows can handle failures.
| ...(fileType === 'image' && { | ||
| image_dimensions: await getHeightAndWidthFromDataUrl(window.URL.createObjectURL(fileToUpload.file)), | ||
| }), |
There was a problem hiding this comment.
Object URL created but never revoked — memory leak per image upload.
window.URL.createObjectURL(fileToUpload.file) allocates a blob URL that persists until explicitly revoked. In a multi-file upload scenario this can accumulate quickly.
🐛 Proposed fix: revoke the object URL after reading dimensions
...(fileType === 'image' && {
- image_dimensions: await getHeightAndWidthFromDataUrl(window.URL.createObjectURL(fileToUpload.file)),
+ image_dimensions: await (async () => {
+ const objectUrl = window.URL.createObjectURL(fileToUpload.file);
+ try {
+ return await getHeightAndWidthFromDataUrl(objectUrl);
+ } finally {
+ window.URL.revokeObjectURL(objectUrl);
+ }
+ })(),
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...(fileType === 'image' && { | |
| image_dimensions: await getHeightAndWidthFromDataUrl(window.URL.createObjectURL(fileToUpload.file)), | |
| }), | |
| ...(fileType === 'image' && { | |
| image_dimensions: await (async () => { | |
| const objectUrl = window.URL.createObjectURL(fileToUpload.file); | |
| try { | |
| return await getHeightAndWidthFromDataUrl(objectUrl); | |
| } finally { | |
| window.URL.revokeObjectURL(objectUrl); | |
| } | |
| })(), | |
| }), |
🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts` around lines 59
- 61, The current call to getHeightAndWidthFromDataUrl uses
window.URL.createObjectURL(fileToUpload.file) but never revokes the created
object URL; modify the upload flow in processMessageUploads.ts so you assign the
result of window.URL.createObjectURL(...) to a const (e.g., blobUrl), pass
blobUrl into getHeightAndWidthFromDataUrl, and then immediately call
window.URL.revokeObjectURL(blobUrl) in a finally block (or after awaiting the
dimensions) to ensure the object URL is always revoked and avoid leaking memory.
| await test.step('send text file again with blacklisted setting set, file upload should fail', async () => { | ||
| await poHomeChannel.content.dragAndDropTxtFile(); | ||
| await poHomeChannel.content.descriptionInput.fill('message 2'); | ||
| await poHomeChannel.content.fileNameInput.fill('any_file2.txt'); | ||
| await poHomeChannel.content.btnModalConfirm.click(); | ||
|
|
||
| await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).toHaveAttribute('readonly'); | ||
| await expect(poHomeChannel.content.lastUserMessage.locator('.rcx-icon--name-key')).toBeVisible(); | ||
| await expect(poHomeChannel.content.getFileDescription).toHaveText('message 2'); | ||
| await expect(poHomeChannel.content.lastMessageFileName).toContainText('any_file2.txt'); | ||
| await poHomeChannel.composer.removeFileByName(TEST_FILE_TXT); | ||
| }); |
There was a problem hiding this comment.
Misleading test step name: says "blacklisted" but the preceding change was to the whitelist.
The step at line 103 sets FileUpload_MediaTypeWhiteList to text/plain, but the step name on line 107 says "blacklisted setting set". Consider renaming to accurately reflect that the whitelist restricts encrypted file uploads (which use application/octet-stream).
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts` around
lines 107 - 112, The test step description is misleading: the code sets
FileUpload_MediaTypeWhiteList to "text/plain" but the test.step message says
"blacklisted setting set"; update the test.step description string in the
e2ee-file-encryption.spec.ts (the test wrapped by test.step and using
poHomeChannel methods like content.dragAndDropTxtFile, composer.getFileByName,
composer.removeFileByName) to reference the whitelist restriction (e.g.,
"whitelist set to text/plain, encrypted file upload should fail") so it
accurately reflects that a whitelist (not a blacklist) is being applied.
| await poHomeChannel.content.inputFileUploadName.fill(updatedFileName); | ||
| await poHomeChannel.content.btnUpdateFileUpload.click(); | ||
|
|
||
| expect(poHomeChannel.composer.getFileByName(updatedFileName)); |
There was a problem hiding this comment.
expect() without an assertion method is a no-op — file visibility is not actually verified.
expect(poHomeChannel.composer.getFileByName(updatedFileName)) evaluates to a locator wrapper but never calls an assertion like .toBeVisible(). This line passes regardless of whether the file is present. The same issue appears on lines 63, 64, 118, and 119.
🐛 Proposed fix for all occurrences
- expect(poHomeChannel.composer.getFileByName(updatedFileName));
+ await expect(poHomeChannel.composer.getFileByName(updatedFileName)).toBeVisible();Apply the same pattern to lines 63, 64, 118, and 119:
- expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT));
- expect(poHomeChannel.composer.getFileByName(TEST_FILE_LST));
+ await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).toBeVisible();
+ await expect(poHomeChannel.composer.getFileByName(TEST_FILE_LST)).toBeVisible();As per coding guidelines: "Use expect matchers for assertions (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements" and "Prefer web-first assertions (toBeVisible, toHaveText, etc.)".
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/file-upload.spec.ts` at line 52, Replace no-op expect
calls that pass a locator wrapper (e.g.,
poHomeChannel.composer.getFileByName(updatedFileName)) with real Playwright
assertions: await
expect(poHomeChannel.composer.getFileByName(updatedFileName)).toBeVisible() (or
another appropriate matcher like toHaveText/toBeAttached) for each occurrence
(the ones at the current diff and the other occurrences referenced around lines
63, 64, 118, 119) so the test actually verifies file presence/visibility.
| test('should respect the maximum number of files allowed per message: 10', async () => { | ||
| const file11 = 'number6.png'; | ||
| const files = new Array(10).fill('number1.png'); | ||
|
|
||
| await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file))); | ||
| await poHomeChannel.content.dragAndDropTxtFile(); | ||
|
|
||
| await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10); | ||
| await expect(poHomeChannel.composer.getFileByName(file11)).not.toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Max-files test asserts the wrong file name — assertion is trivially true.
file11 is 'number6.png', but the 11th file actually attempted is via dragAndDropTxtFile() which drops 'any_file.txt'. The assertion on line 152 checks that 'number6.png' is not visible, which always passes since that file was never uploaded. The check should verify that the drag-and-dropped txt file was rejected.
🐛 Proposed fix
test('should respect the maximum number of files allowed per message: 10', async () => {
- const file11 = 'number6.png';
const files = new Array(10).fill('number1.png');
await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file)));
await poHomeChannel.content.dragAndDropTxtFile();
await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10);
- await expect(poHomeChannel.composer.getFileByName(file11)).not.toBeVisible();
+ await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).not.toBeVisible();
});🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/file-upload.spec.ts` around lines 144 - 153, The test
"should respect the maximum number of files allowed per message: 10" is
asserting the wrong filename; replace the current assertion that checks for
'number6.png' with a check that the actual drag-and-dropped file (the txt file
dropped by poHomeChannel.content.dragAndDropTxtFile) was rejected — i.e.,
capture or use the txt filename (e.g., 'any_file.txt' or the value returned/used
by dragAndDropTxtFile) and assert
poHomeChannel.composer.getFileByName(<txt-file-name>) is not visible while still
asserting getFilesInComposer() has count 10; update the reference in the
assertion accordingly.
| export default async function waitForMediaResponse(page: Page) { | ||
| let responsePromise; | ||
| try { | ||
| responsePromise = page.waitForResponse((response) => isMediaResponse(response)); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
|
|
||
| return responsePromise; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The try/catch is ineffective — page.waitForResponse won't throw synchronously.
page.waitForResponse() returns a Promise immediately; it doesn't throw synchronously. The try/catch here only guards the promise creation, not its resolution. Any actual timeout or error will surface when the caller awaits the returned promise, bypassing this catch entirely.
Simplify to return the promise directly:
♻️ Suggested fix
-export default async function waitForMediaResponse(page: Page) {
- let responsePromise;
- try {
- responsePromise = page.waitForResponse((response) => isMediaResponse(response));
- } catch (error) {
- console.error(error);
- }
-
- return responsePromise;
+export default function waitForMediaResponse(page: Page) {
+ return page.waitForResponse((response) => isMediaResponse(response));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default async function waitForMediaResponse(page: Page) { | |
| let responsePromise; | |
| try { | |
| responsePromise = page.waitForResponse((response) => isMediaResponse(response)); | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| return responsePromise; | |
| export default function waitForMediaResponse(page: Page) { | |
| return page.waitForResponse((response) => isMediaResponse(response)); | |
| } |
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/fixtures/responses/mediaResponse.ts` around lines 5 -
13, The current try/catch in waitForMediaResponse is ineffective because
page.waitForResponse returns a Promise and won't throw synchronously; remove the
try/catch and simply return the promise from page.waitForResponse((response) =>
isMediaResponse(response)) directly in the waitForMediaResponse function so
errors/timeouts surface to the caller when they await the returned promise.
| confirmTemporaryFiles(fileIds: string[], userId: string): Promise<Document | UpdateResult> | undefined; | ||
|
|
There was a problem hiding this comment.
Interface declares | undefined but the implementation never returns undefined.
confirmTemporaryFiles is typed as returning Promise<…> | undefined, matching the single-file confirmTemporaryFile signature. However, unlike confirmTemporaryFile (which historically had an early-return guard on empty fileId), the batch implementation in BaseUploadModel.ts always returns this.updateMany(…). Consider either adding an empty-array guard in the implementation or removing | undefined from this signature for accuracy.
🤖 Prompt for AI Agents
In `@packages/model-typings/src/models/IBaseUploadsModel.ts` around lines 13 - 14,
The interface method confirmTemporaryFiles currently allows a return of
undefined but the BaseUploadModel.confirmTemporaryFiles implementation always
returns this.updateMany(...); fix by making the signature and implementation
consistent: either remove the "| undefined" from confirmTemporaryFiles in
IBaseUploadsModel to return Promise<Document | UpdateResult> (preferred if you
want no early-exit), or add an empty-array guard to
BaseUploadModel.confirmTemporaryFiles that returns undefined when fileIds.length
=== 0 to match the single-file confirmTemporaryFile behavior; update the
signature or implementation accordingly so the declared return type matches the
actual return value.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 121 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/client/lib/chats/flows/uploadFiles.ts">
<violation number="1" location="apps/meteor/client/lib/chats/flows/uploadFiles.ts:67">
P2: If e2eRoom.encryptFile(file) fails (returns null/undefined), the queue stops because uploadNextFile() is only called from uploadFile(), which is invoked only on successful encryption. This silently stalls remaining uploads.</violation>
</file>
<file name="apps/meteor/client/lib/chats/flows/processMessageUploads.ts">
<violation number="1" location="apps/meteor/client/lib/chats/flows/processMessageUploads.ts:19">
P2: getHeightAndWidthFromDataUrl never resolves on image load failure, so awaiting it can hang indefinitely for corrupt/failed images.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| sha256: encryptedFile.hash, | ||
| }, | ||
| }; | ||
| if (encryptedFile) { |
There was a problem hiding this comment.
P2: If e2eRoom.encryptFile(file) fails (returns null/undefined), the queue stops because uploadNextFile() is only called from uploadFile(), which is invoked only on successful encryption. This silently stalls remaining uploads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/flows/uploadFiles.ts, line 67:
<comment>If e2eRoom.encryptFile(file) fails (returns null/undefined), the queue stops because uploadNextFile() is only called from uploadFile(), which is invoked only on successful encryption. This silently stalls remaining uploads.</comment>
<file context>
@@ -1,206 +1,93 @@
- sha256: encryptedFile.hash,
- },
- };
+ if (encryptedFile) {
+ const fileContentData = {
+ type: file.type,
</file context>
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/room/composer/ComposerMessage.tsx (1)
23-23: 🛠️ Refactor suggestion | 🟠 MajorRemove the unused
onUploadFilesprop fromComposerMessageProps.The prop is defined in the type but is not accepted by
MessageBox(missing fromMessageBoxProps). It has no usage anywhere in the codebase. Passing it via...propsintroduces unnecessary type pollution; remove it to clarify the actual API.apps/meteor/client/views/room/body/hooks/useFileUploadDropTarget.ts (1)
50-61:⚠️ Potential issue | 🟠 MajorBug: File deduplication by size alone drops distinct files of equal size.
Using only
file.sizeas the dedup key means two genuinely different files that happen to share the same byte count (very common — e.g., two 0-byte files, two small text files, screenshots at the same resolution) will be silently dropped. This should use a composite key or a different strategy.A more robust approach would combine
file.name,file.size, andfile.lastModified:Proposed fix
const getUniqueFiles = () => { const uniqueFiles: File[] = []; - const st: Set<number> = new Set(); + const st: Set<string> = new Set(); files.forEach((file) => { - const key = file.size; + const key = `${file.name}-${file.size}-${file.lastModified}`; if (!st.has(key)) { uniqueFiles.push(file); st.add(key); } }); return uniqueFiles; };
🤖 Fix all issues with AI agents
In `@apps/meteor/app/lib/server/methods/sendMessage.ts`:
- Around line 155-183: The filesToConfirm flow lacks ownership checks: ensure
updateFileContentById enforces the current user owns the upload before changing
encrypted content (add userId filter/validation inside updateFileContentById and
reject if not owned), make findByIds calls in the upload processing include
userId filtering (or replace with a findByIdsAndUserId helper) so only the
caller's uploads are returned, and after calling updateFileComplete verify its
return value indicates a successful update (throw/abort if no document was
modified) before proceeding. Reference updateFileContentById, findByIds,
updateFileComplete and filesToConfirm when making these changes.
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts`:
- Around line 59-61: The code creates a blob URL via
window.URL.createObjectURL(fileToUpload.file) for getting image dimensions but
never revokes it; update the call site (where getHeightAndWidthFromDataUrl is
invoked) to assign the blob URL to a variable, pass that variable into
getHeightAndWidthFromDataUrl, and ensure you call
window.URL.revokeObjectURL(blobUrl) after the dimensions are retrieved (use a
try/finally around the await to guarantee revoke even on error) so the blob URL
is always revoked and avoids memory leaks.
- Around line 155-178: In continueSendingMessage the encryption step uses the
original filesToUpload (cast to EncryptedUpload[]) while the loop builds
fileUrls and filesToConfirm only from uploads that have url and id; filter
filesToUpload the same way before calling getEncryptedContent so the encrypted
payload matches the actually-sent files—e.g., create a filteredUploads (only
uploads with upload.url && upload.id, and include metadataForEncryption when
required) and pass that filteredUploads to getEncryptedContent and any other
encryption logic (keeping references to continueSendingMessage, filesToUpload,
fileUrls, filesToConfirm, getEncryptedContent, EncryptedUpload, e2eRoom,
shouldConvertSentMessages).
- Around line 16-27: The promise in getHeightAndWidthFromDataUrl can hang if the
Image fails to load; add an img.onerror handler that rejects the Promise with an
Error (e.g., "Failed to load image") and ensure you attach error and load
handlers before setting img.src; also perform minimal cleanup inside both
handlers (clear onload/onerror) so resolve/reject only happens once and the
promise never remains pending on broken/invalid image data.
In `@apps/meteor/client/lib/chats/flows/uploadFiles.ts`:
- Around line 52-58: The early-return paths in uploadFiles.ts (the
settings.peek('E2E_Enable_Encrypt_Files') check and the branch where encryptFile
returns falsy or throws) currently stop processing the remaining queue; update
those error paths to either call uploadNextFile() to continue the queue or
explicitly surface the failure before returning. Concretely, in the block
guarded by settings.peek('E2E_Enable_Encrypt_Files') (where dispatchToastMessage
is called) and in the encryptFile failure/false branch (and its catch), after
dispatching the error toast or handling the failed encryption, invoke
uploadNextFile() (or rethrow if you want the caller to handle it) so remaining
files are not silently dropped; reference uploadNextFile, encryptFile,
dispatchToastMessage, and t when locating the spots to change.
- Around line 23-31: The uploadFile function dispatches uploadsStore.send(file,
encrypted) without awaiting it, causing all files to start in parallel; make
uploadFile async and await uploadsStore.send(...) before calling
uploadNextFile() so the queue processes uploads sequentially (update the
uploadFile declaration and its callers if needed to handle the async return).
Ensure you reference uploadFile, uploadsStore.send, and uploadNextFile when
making the change so callers that expect synchronous behavior are updated.
In `@apps/meteor/client/lib/chats/uploads.ts`:
- Around line 98-111: The Promise executor in uploads.ts (inside the async block
that creates new Promise) calls reject(...) for invalid files but does not
return, so execution continues and triggers the XHR upload; update the Promise
executor used in the upload flow to immediately return after each reject call
(e.g., after checks for file.size === 0, maxFileSize exceedance, and
invalidContentType) to prevent further processing and network requests, and
ensure no subsequent resolve (such as from the load callback) can be reached for
already-rejected inputs.
- Around line 162-198: xhr.onload currently calls JSON.parse(xhr.responseText)
twice (in the 400 and 200 branches) without protection, so a malformed response
can throw inside the event handler and leave uploads stuck; wrap the entire
xhr.onload body in a try-catch and on any parse or runtime error call this.set
to update the matching upload (by id) with an Error (e.g., new Error(parsed or
generic message)) so the UI is notified, and ensure the catch also handles
non-JSON responses; locate the xhr.onload handler, the JSON.parse calls, and the
this.set(...) blocks to implement the try-catch around them.
In `@apps/meteor/client/providers/ImageGalleryProvider.tsx`:
- Around line 26-36: Remove the multi-line explanatory comment and instead open
an issue to track the real root cause in RoomMessageContent; in the handler
replace the fragile string-split idFromSrc logic with a clearer, more robust
extraction (e.g., use a named-group regex or URL parsing) and keep fallback
behavior that prefers target.dataset.id or the closest '.gallery-item-container'
data-id before calling setImageId; reference idFromSrc, target.dataset.src,
target.dataset.id, the closest('.gallery-item-container') data-id, setImageId,
and RoomMessageContent so reviewers can locate the code and the upstream bug to
track.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageComposerFileComponent.tsx`:
- Around line 20-22: The CSS value 'surface-tint' in the previewWrapperStyle is
an invalid literal string; replace the raw string with the resolved token value
(use Palette.surface['surface-tint'] like you already do for
Palette.surface['surface-hover']) so the non-hover background actually applies,
ensuring previewWrapperStyle references the Palette.surface token (and
import/namespace for Palette if missing).
In
`@apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerModal.tsx`:
- Line 147: handleUpload currently calls the async uploadFile(file) without
awaiting it; change handleUpload to be async and await uploadFile(file) so the
component waits for uploadFile's finally block (which calls setIsLoading(false)
and onClose()) before returning, preventing state updates on an unmounted
component and allowing callers to track the upload lifecycle; update any callers
or event handlers (e.g., button onClick) to handle the returned promise if
needed.
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts`:
- Around line 118-125: The assertion is vacuous because
composerFilesLocator.all() (composerFiles) is resolved before calling
poHomeChannel.content.dragAndDropTxtFile(), so if no matching files exist it
returns an empty array; fix by calling composerFilesLocator.all() after
poHomeChannel.content.dragAndDropTxtFile() (or assert composerFiles.length > 0
immediately after resolving) and then run Promise.all(...) to check the readonly
attribute on each file; also ensure the test still checks
poHomeChannel.content.lastUserMessage.locator('.rcx-icon--name-key') afterwards.
In `@apps/meteor/tests/e2e/file-upload.spec.ts`:
- Line 52: The test currently calls
expect(poHomeChannel.composer.getFileByName(updatedFileName)) with no matcher,
which is a no-op; update each such call (including
poHomeChannel.composer.getFileByName(updatedFileName) and the other occurrences
on lines 63, 64, 118, 119) to use an appropriate Playwright matcher (e.g.,
append .toBeTruthy(), .toBeDefined(), or a UI-focused assertion like
.toBeVisible()/toHaveText() depending on whether getFileByName returns an
element or a data value) so the expectation actually asserts the intended
condition; locate the calls via poHomeChannel.composer.getFileByName and replace
them with expect(...).<chosenMatcher>().
- Around line 144-153: The test uses a misleading file11 ('number6.png') and
thus the not.toBeVisible() assertion is vacuous; update the test in the "should
respect the maximum number of files allowed per message: 10" case to reference
the actual 11th filename produced by poHomeChannel.content.dragAndDropTxtFile()
(or use the TEST_FILE_TXT constant used by that helper) instead of
'number6.png', and assert that
poHomeChannel.composer.getFileByName(<actual-11th-filename>) is not visible (and
keep the getFilesInComposer count assertion) so the test verifies the dropped
TXT file was rejected due to the 10-file limit.
🟡 Minor comments (12)
apps/meteor/tests/e2e/e2e-encryption/e2ee-encryption-decryption.spec.ts-99-107 (1)
99-107:⚠️ Potential issue | 🟡 Minor
sendMessageinsideexpect().toPass()makes the retry block non-idempotent.If
sendMessage(fileDescription)on Line 106 succeeds buttoPass()retries due to a transient failure earlier in the chain (or a timeout at the block level), the message could be sent twice, causing subsequent assertions to match the wrong message. MovesendMessageoutside the retry block so only the idempotent rename/update flow is retried.Proposed fix
await test.step('upload the file with encryption', async () => { // Upload a file await encryptedRoomPage.dragAndDropTxtFile(); // Update file name and send await expect(async () => { await encryptedRoomPage.composer.getFileByName('any_file.txt').click(); await fileUploadModal.setName(fileName); await fileUploadModal.update(); await expect(encryptedRoomPage.composer.getFileByName(fileName)).toBeVisible(); - await encryptedRoomPage.sendMessage(fileDescription); }).toPass(); + await encryptedRoomPage.sendMessage(fileDescription);apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx-50-50 (1)
50-50:⚠️ Potential issue | 🟡 MinorAdd the missing "Update" translation to the test mock.
The mock translations (lines 22–28) don't include an "Update" key, but the component button now renders
t('Update')(line 103). While the test likely passes due to translation fallback behavior, explicitly mocking the translation ensures the test is not fragile to translation library changes. AddUpdate: 'Update'to the mock on line 22–28.apps/meteor/tests/e2e/fixtures/responses/mediaResponse.ts-5-13 (1)
5-13:⚠️ Potential issue | 🟡 MinorThe
try/catchsilently swallows errors and makes the return typeundefined-able.
page.waitForResponse()returns aPromise— it doesn't throw synchronously. Thetry/catchis effectively dead code. If it did catch (e.g., page is disposed), the error is logged but swallowed, andresponsePromisebecomesundefined. Callers awaiting the result would then getundefinedinstead of aResponse, leading to confusing downstream failures.Since this is a test utility, let errors propagate so failures are immediately visible.
🔧 Proposed fix
export default async function waitForMediaResponse(page: Page) { - let responsePromise; - try { - responsePromise = page.waitForResponse((response) => isMediaResponse(response)); - } catch (error) { - console.error(error); - } - - return responsePromise; + return page.waitForResponse((response) => isMediaResponse(response)); }apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts-157-161 (1)
157-161:⚠️ Potential issue | 🟡 MinorInconsistent use of
TEST_FILE_TXTconstant.Line 160 uses the hardcoded string
'any_file.txt'instead of theTEST_FILE_TXTconstant defined on Line 18. Use the constant for consistency.Proposed fix
- await expect(poHomeChannel.composer.getFileByName('any_file.txt')).not.toBeVisible(); + await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).not.toBeVisible();apps/meteor/client/lib/chats/flows/processMessageUploads.ts-101-153 (1)
101-153:⚠️ Potential issue | 🟡 MinorDead code on Line 153 — unreachable
return continueSendingMessage().The conditional flow is:
- Line 101–103:
filesToUpload.length === 0→ return- Line 107–109:
!failedUploads.length→ returncontinueSendingMessage()- Line 111–150:
failedUploads.length > 0→ return promise (always true at this point)Line 153 is never reached. Remove it to avoid confusion.
Proposed fix
}); } - - return continueSendingMessage(); async function continueSendingMessage() {apps/meteor/app/ui/client/lib/ChatMessages.ts-126-128 (1)
126-128:⚠️ Potential issue | 🟡 MinorAdd upload cleanup to
release()methodThe
editMessagemethod correctly clears the appropriate upload store based on thread context (line 127), butrelease()(lines 195-203) does not calluploads.clear()orthreadUploads.clear(). When a user navigates away while files are queued, the stores retain references to File objects until garbage collection. Add the same clear logic torelease()to ensure proper cleanup when switching contexts.apps/meteor/app/lib/server/functions/sendMessage.ts-339-341 (1)
339-341:⚠️ Potential issue | 🟡 MinorTemporary files are confirmed after message persistence — failure leaves orphaned references.
If
Uploads.confirmTemporaryFilesthrows, the message is already persisted withfiles/attachmentsreferencing uploads that remain in a temporary state. Consider wrapping this in a try-catch that logs the error rather than propagating it, or moving confirmation to a transactional boundary with the message insert.apps/meteor/client/lib/chats/flows/sendMessage.ts-67-68 (1)
67-68:⚠️ Potential issue | 🟡 MinorPotential TOCTOU:
hasFileschecked beforeprocess()reads the store.
hasFilesis computed at line 68 fromuploadsStore.get().length > 0, but by the timeprocessMessageUploadsruns (line 29), the store may have changed (e.g., user removed all files between composition and send). This would causehasFilesto betruewhileprocessMessageUploadsfinds no files and returnsfalse, then the flow continues tosdk.call('sendMessage')with an emptymsg.In practice, this is a very narrow race window in single-threaded JS, and the worst case is sending an empty message. But it may be worth a defensive check inside
processor re-evaluatinghasFilescloser to its consumption.apps/meteor/client/views/room/body/hooks/useFileUpload.ts-34-34 (1)
34-34:⚠️ Potential issue | 🟡 Minor
isUploadingincludes errored uploads — likely a bug.
uploads.some((upload) => upload.percentage < 100)will returntruefor uploads that have failed with an error (error set, percentage < 100). This could keep the UI in an "uploading" state even when all active uploads have either completed or failed.Proposed fix
- const isUploading = uploads.some((upload) => upload.percentage < 100); + const isUploading = uploads.some((upload) => upload.percentage < 100 && !upload.error);apps/meteor/client/views/room/composer/messageBox/MessageComposerFile.tsx-64-64 (1)
64-64:⚠️ Potential issue | 🟡 MinorMissing
e.preventDefault()for Space key may cause page scroll.When Space is pressed on a focusable element, browsers scroll the page by default. Without calling
preventDefault(), the file preview opens and the page scrolls, degrading the keyboard-navigation experience.🛡️ Proposed fix
- onKeyDown={(e) => ['Enter', 'Space'].includes(e.code) && handleOpenFilePreview()} + onKeyDown={(e) => { + if (['Enter', 'Space'].includes(e.code)) { + e.preventDefault(); + handleOpenFilePreview(); + } + }}apps/meteor/client/lib/chats/flows/uploadFiles.ts-90-92 (1)
90-92:⚠️ Potential issue | 🟡 Minor
uploadNextFile()result is fire-and-forget — unhandled rejections possible.
uploadNextFile()is async but its returned promise is not awaited or caught at Line 90. Any error thrown inside the upload chain (e.g.,e2eRoom.encryptFilefailure) becomes an unhandled promise rejection.Proposed fix
- uploadNextFile(); + uploadNextFile().catch((error) => { + dispatchToastMessage({ type: 'error', message: String(error) }); + });apps/meteor/client/lib/chats/uploads.ts-34-36 (1)
34-36:⚠️ Potential issue | 🟡 MinorCancel won't work after upload succeeds because the store ID changes.
At line 191, on a successful upload, the entry's
idis replaced withresult.file._id. Butcancel(id)at line 34 emitscancelling-${id}. Theoncelistener at line 200 is still keyed to the original random ID. So callingcancel()with the new server-assigned ID (which is what the UI would see) won't trigger the listener, and thexhr.abort()won't execute.If cancellation of an already-uploaded-but-not-yet-sent file should remove it from the store, consider using
removeUploadfor that case, or re-registering the listener with the updated ID.Also applies to: 200-204
🧹 Nitpick comments (20)
apps/meteor/client/views/room/composer/messageBox/MessageComposerFileLoader.tsx (1)
5-19: Consider hoisting the CSS class outside the component.The
csstemplate literal is evaluated on every render. MovingcustomCSSoutside the component body avoids re-computation and potential class re-generation per render cycle.Also, the
TODOcomment on line 5 is a code comment in implementation. As per coding guidelines, avoid code comments in the implementation.♻️ Proposed refactor
+const spinnerCSS = css` + animation: spin-animation 0.8s linear infinite; + + `@keyframes` spin-animation { + from { + transform: rotate(0deg); + } + + to { + transform: rotate(360deg); + } + } +`; + -// TODO: This component should be moved to fuselage const MessageComposerFileLoader = (props: ComponentProps<typeof Box>) => { - const customCSS = css` - animation: spin-animation 0.8s linear infinite; - - `@keyframes` spin-animation { - from { - transform: rotate(0deg); - } - - to { - transform: rotate(360deg); - } - } - `; - return ( - <Box className={customCSS} is='svg' size='x24' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg' {...props}> + <Box className={spinnerCSS} is='svg' size='x24' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg' {...props}>apps/meteor/tests/end-to-end/api/moderation.ts (2)
773-845: Test cleanup: noafterhook if the test assertion fails early.If the
beforeblock succeeds but theitblock fails beforedeleteReportedMessagesruns, the uploaded files and message remain in the "general" channel with no cleanup. Consider adding anafterhook that attempts to deletemessageWithFilesto keep the test environment clean for parallel/subsequent runs.♻️ Proposed addition
+ after(async () => { + if (messageWithFiles?._id) { + await request + .post(api('chat.delete')) + .set(credentials) + .send({ roomId: generalRoomId, msgId: messageWithFiles._id }); + } + }); + it('should delete reported messages and all associated files', async () => {
841-843:for awaitis unnecessary on a synchronous array.
fileUrlsis a plainstring[]. A regularfor...ofloop is sufficient and more idiomatic.♻️ Proposed fix
- for await (const fileUrl of fileUrls) { + for (const fileUrl of fileUrls) { await request.get(fileUrl).set(credentials).expect(404); }apps/meteor/client/lib/chats/flows/processMessageUploads.ts (1)
191-205: Parallel sends withPromise.allmay cause message ordering issues.When multi-file-per-message is disabled (Line 192), each file is sent as a separate message via
Promise.all. Since these are concurrent, the server may process them out of order, resulting in files appearing in an unpredictable sequence in the chat. Consider using sequential sends (for...ofloop withawait) to preserve the user's intended file order.apps/meteor/app/file-upload/server/methods/sendFileMessage.ts (1)
26-37: Validation checks field presence but not value types.
validateFilesRequiredFieldsonly checks that keys exist viaObject.keys(file).includes(field). A file with{ _id: null, name: undefined, type: '', size: 0 }would pass validation even though_idandnameare falsy. Consider also validating that required fields have truthy/valid values, especially_idandname.apps/meteor/client/views/room/composer/messageBox/MessageComposerFileComponent.tsx (1)
14-27:csstemplate literals inside the component body are re-evaluated on every render.Both
closeWrapperStyleandpreviewWrapperStyleare created inside the function body, generating new class names each render cycle. SincecloseWrapperStyleis static andpreviewWrapperStyleonly depends onerror, consider hoisting the static one outside the component and memoizing the dynamic one.apps/meteor/tests/e2e/page-objects/fragments/composer.ts (1)
32-34: Consider usingexact: trueto avoid ambiguous matches when file names share substrings.
getByRole('button', { name: fileName })does a substring match by default. If two files share a common prefix (e.g.,file.txtandfile.txt.bak), this could match the wrong element.Proposed fix
getFileByName(fileName: string): Locator { - return this.root.getByRole('button', { name: fileName }); + return this.root.getByRole('button', { name: fileName, exact: true }); }apps/meteor/tests/e2e/page-objects/fragments/home-content.ts (1)
341-373: Inconsistent media-wait option across drag-and-drop helpers.
dragAndDropLstFileanddragAndDropTxtFileToThreadaccept optional wait-for-media flags, butdragAndDropTxtFile(line 341) does not. If callers later need to skip the wait (as with the other two), this will require a breaking change. Consider adding the same optional parameter for consistency.♻️ Suggested change
-async dragAndDropTxtFile(): Promise<void> { +async dragAndDropTxtFile({ waitForLoad = true }: { waitForLoad?: boolean } = {}): Promise<void> { const contract = await fs.readFile(getFilePath('any_file.txt'), 'utf-8'); ... await this.page.locator('[role=dialog][data-qa="DropTargetOverlay"]').dispatchEvent('drop', { dataTransfer }); + if (waitForLoad) { + await waitForMediaResponse(this.page); + } }apps/meteor/app/lib/server/functions/sendMessage.ts (2)
237-250: Growing positional parameter list — consider an options object.
sendMessagenow has 6 positional parameters, making call sites fragile and harder to read (callers must remember the order ofupsert,previewUrls,filesToConfirm). An options object for the trailing optional parameters would improve maintainability.♻️ Suggested approach
-export const sendMessage = async ( - user: any, - message: any, - room: any, - upsert = false, - previewUrls?: string[], - filesToConfirm?: IUploadToConfirm[], -) => { +export const sendMessage = async ( + user: any, + message: any, + room: any, + options: { upsert?: boolean; previewUrls?: string[]; filesToConfirm?: IUploadToConfirm[] } = {}, +) => { + const { upsert = false, previewUrls, filesToConfirm } = options;This would require updating all call sites, so it can be deferred if the scope is too large for this PR.
221-235:updateFileNamessilently ignores entries with nonameand nocontent.If a
filesToConfirmentry has neithernamenorcontent, thePromise.allmap returnsundefinedfor that entry. This is technically fine (no side effects), but it means a malformed entry is silently skipped. If this is intentional, it's acceptable; if not, you may want to log or validate.apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
384-384: Confusing variable name — consider renaming for clarity.
shouldDisableDueUploadsis!hasUploads || isUploading, meaning "there are no ready-to-send uploads." The name reads like a boolean for "should we disable because of uploads" but it's actually "should we disable because there's nothing upload-related to send." A name likehasNoReadyUploadsor inverting toconst hasReadyUploads = hasUploads && !isUploadingwould reduce confusion at the three usage sites (lines 486, 488, 489).♻️ Suggested rename
-const shouldDisableDueUploads = !hasUploads || isUploading; +const hasReadyUploads = hasUploads && !isUploading;Then at usage sites:
-disabled={!canSend || (!typing && !isEditing && shouldDisableDueUploads)} +disabled={!canSend || (!typing && !isEditing && !hasReadyUploads)} ... -secondary={typing || isEditing || !shouldDisableDueUploads} +secondary={typing || isEditing || hasReadyUploads} ... -info={typing || isEditing || !shouldDisableDueUploads} +info={typing || isEditing || hasReadyUploads}apps/meteor/client/lib/chats/Upload.ts (1)
18-24:EncryptedFilefields are mutable — consider making themreadonly.
NonEncryptedUploadcorrectly marks its fieldsreadonly, butEncryptedFilefields (file,key,iv,type,hash) are mutable. SinceEncryptedUploadmarks the reference asreadonly encryptedFile, the nested properties can still be inadvertently mutated. If these represent immutable encryption artifacts (which they should be — an IV or key shouldn't change after creation),readonlywould be appropriate.♻️ Suggested change
export type EncryptedFile = { - file: File; - key: JsonWebKey; - iv: string; - type: File['type']; - hash: string; + readonly file: File; + readonly key: JsonWebKey; + readonly iv: string; + readonly type: File['type']; + readonly hash: string; };apps/meteor/client/views/room/composer/messageBox/MessageComposerFile.tsx (1)
32-44: FileUploadModal opened for rename only — modalfileprop may trigger unnecessary preview rendering.
file={upload.file}is passed toFileUploadModal, but the modal is used here solely for renaming. If the modal renders a full file preview (image/video), this could be expensive for large files or unexpected for non-previewable types. Ensure this is the intended behavior and that the modal gracefully handles all file types.apps/meteor/app/lib/server/methods/sendMessage.ts (1)
110-110: Consider using an options object forsendMessage— 6 positional parameters is fragile.
sendMessage(user, message, room, false, extraInfo?.previewUrls, extraInfo?.filesToConfirm)is now at 6 positional arguments, making call sites brittle and hard to read. This is a pre-existing pattern, but the addition offilesToConfirmis a good opportunity to refactor toward a single options object for the optional trailing parameters.apps/meteor/client/views/room/body/hooks/useFileUpload.ts (1)
7-12: Runtime guard forstoreis redundant — parameter is non-optional.
storeis typed asUploadsAPI(notUploadsAPI | undefined), so the!storecheck at Line 10 is unreachable under correct TypeScript usage. If you want to defend against JS callers, that's fine, but it's worth noting the redundancy.apps/meteor/client/views/composer/AudioMessageRecorder/AudioMessageRecorder.tsx (1)
84-84:chatdeclared afteruseEffectEventcallbacks that reference it.
stopRecording(Line 29),handleRecord(Line 53), and otheruseEffectEventcallbacks referencechat, which is declared later at Line 84. This works at runtime becauseuseEffectEventdefers closure capture, but it reduces readability. Consider movingconst chat = useChat()abovestopRecording.This predates this PR but was touched here (removed the
?? chatContextfallback).apps/meteor/client/lib/chats/uploads.ts (4)
113-160: Dual settlement paths (loadcallback +xhr.onload) are fragile and hard to reason about.The
loadcallback at line 122 resolves the Promise on any completed HTTP response (including 4xx), whilexhr.onloadat line 162 separately processes status codes and sets error state. This means:
- For a 400 response: the Promise resolves successfully (via
loadcallback), so thecatchat line 206 is never reached, and the error is only reflected through the state update inonload. This works, but it's indirect and makes it easy for future maintainers to misunderstand the error flow.- For a 200 response: both callbacks fire —
resolveand the id/url state update — which works but is fragile.Consider consolidating all response handling into a single callback to avoid the split control flow.
Also applies to: 162-198
104-104: Remove code comment per coding guidelines.As per coding guidelines,
**/*.{ts,tsx,js}: "Avoid code comments in the implementation."Proposed fix
- // -1 maxFileSize means there is no limit if (maxFileSize > -1 && (file.size || 0) > maxFileSize) {
46-78:editUploadFileNameis markedasyncbut performs no async work.The method is declared
asyncbut contains noawait. TheUploadsAPIinterface defines it as returningvoid, notPromise<void>. While TypeScript allows assigningasync () => Promise<void>to() => void, theasynckeyword is misleading here — callers won'tawaitthe result (since the interface saysvoid), so any thrown error would become an unhandled promise rejection rather than being caught.In practice the internal try-catch handles all possible throws, so this is safe today, but removing
asyncmakes the intent clearer.Proposed fix
- editUploadFileName = async (uploadId: Upload['id'], fileName: Upload['file']['name']): Promise<void> => { + editUploadFileName = (uploadId: Upload['id'], fileName: Upload['file']['name']): void => {
200-204: Cancellation listener is not cleaned up on normal completion.The
this.once(\cancelling-${id}`)listener registered at line 200 is never removed if the upload completes successfully or fails via the error callback. Since the uploadidin the store is replaced with the server-assigned ID on success (line 191), this orphaned listener for the original random ID will never fire and will linger until theUploadsStore` instance is garbage collected. For a long-lived room with many uploads this could accumulate.Consider removing the listener in both the
loadanderrorpaths.
| check( | ||
| filesToConfirm, | ||
| Match.Maybe([ | ||
| Match.ObjectIncluding({ | ||
| _id: String, | ||
| name: Match.Maybe(String), | ||
| content: Match.Maybe( | ||
| Match.OneOf( | ||
| { | ||
| algorithm: 'rc.v1.aes-sha2', | ||
| ciphertext: String, | ||
| }, | ||
| { | ||
| algorithm: 'rc.v2.aes-sha2', | ||
| ciphertext: String, | ||
| kid: String, | ||
| iv: String, | ||
| }, | ||
| ), | ||
| ), | ||
| }), | ||
| ]), | ||
| ); | ||
|
|
||
| if (filesToConfirm && filesToConfirm.length > MAX_MULTIPLE_UPLOADED_FILES) { | ||
| throw new Meteor.Error('error-too-many-files', `Cannot send more than ${MAX_MULTIPLE_UPLOADED_FILES} files in one message`, { | ||
| method: 'sendMessage', | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the sendMessage function implementation
fd -e ts -e js | grep -i sendmessage | head -20Repository: RocketChat/Rocket.Chat
Length of output: 756
🏁 Script executed:
# Search for the sendMessage function definition in the codebase
rg -n 'export.*function sendMessage' --type=ts -A5Repository: RocketChat/Rocket.Chat
Length of output: 1560
🏁 Script executed:
# Look for filesToConfirm usage throughout the codebase
rg -n 'filesToConfirm' --type=ts -B2 -A5Repository: RocketChat/Rocket.Chat
Length of output: 15736
🏁 Script executed:
# Search for ownership validation patterns related to uploads
rg -n 'owner|userId|user.*id.*upload|upload.*owner' --type=ts -i -g '**/sendMessage*' -g '**/sendFileMessage*' -C3Repository: RocketChat/Rocket.Chat
Length of output: 6320
🏁 Script executed:
# Let's examine the Uploads.findByIds implementation and updateFileComplete
rg -n 'findByIds|updateFileComplete' --type=ts -B2 -A8 -g '**/server/**'Repository: RocketChat/Rocket.Chat
Length of output: 25989
🏁 Script executed:
# Check if there's any ownership/userId validation when fetching uploads
rg -n 'userId|ownerId|user.*_id' --type=ts apps/meteor/app/file-upload/server/methods/sendFileMessage.ts -B3 -A3Repository: RocketChat/Rocket.Chat
Length of output: 1615
🏁 Script executed:
# Look at the Uploads collection schema and methods
fd -name '*Upload*' -type f | grep -E '\.(ts|js)$' | grep -v node_modules | grep -v test | head -10Repository: RocketChat/Rocket.Chat
Length of output: 296
🏁 Script executed:
# Search for Uploads collection definition
rg -n 'export.*Uploads|new Collection' --type=ts -g '**/server/**' -B1 -A3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 790
🏁 Script executed:
# Find the Uploads collection definition and updateFileComplete method
rg -n 'updateFileComplete|Uploads\s*=' --type=ts --type=js -g '**/server/**' -B2 -A10 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 8231
🏁 Script executed:
# Search for the Uploads class/model definition
fd -type f -name '*[Uu]pload*' | grep -E '\.(ts|js)$' | grep -v node_modules | grep -v test | grep -v '.spec'Repository: RocketChat/Rocket.Chat
Length of output: 236
🏁 Script executed:
# Look for the Uploads collection definition more specifically
rg -n 'export.*Uploads|class Uploads' --type=ts -g '**/models/**' -g '**/collections/**'Repository: RocketChat/Rocket.Chat
Length of output: 1012
🏁 Script executed:
# Check if there's validation when fetching uploads by ID
rg -n 'findByIds.*upload' --type=ts -i -C5Repository: RocketChat/Rocket.Chat
Length of output: 1212
🏁 Script executed:
# Check the BaseUploadModel implementation for updateFileComplete
cat packages/models/src/models/BaseUploadModel.tsRepository: RocketChat/Rocket.Chat
Length of output: 3487
🏁 Script executed:
# Check the Uploads model implementation
cat packages/models/src/models/Uploads.tsRepository: RocketChat/Rocket.Chat
Length of output: 2888
🏁 Script executed:
# Look for updateFileComplete method definition in BaseUploadModel
rg -n 'updateFileComplete' --type=ts -B5 -A15Repository: RocketChat/Rocket.Chat
Length of output: 7198
Verify ownership validation of filesToConfirm across the entire flow.
The updateFileComplete method correctly validates upload ownership via userId filtering. However, for E2EE messages, updateFileNames calls updateFileContentById without any ownership validation, allowing a malicious client to modify encrypted content of uploads not belonging to them. Additionally, findByIds lacks ownership checks. Ensure that:
updateFileContentByIdvalidates file ownership before updating encrypted contentfindByIdsin the upload processing flow includes userId filtering- The result of
updateFileCompleteis verified to have actually succeeded
🤖 Prompt for AI Agents
In `@apps/meteor/app/lib/server/methods/sendMessage.ts` around lines 155 - 183,
The filesToConfirm flow lacks ownership checks: ensure updateFileContentById
enforces the current user owns the upload before changing encrypted content (add
userId filter/validation inside updateFileContentById and reject if not owned),
make findByIds calls in the upload processing include userId filtering (or
replace with a findByIdsAndUserId helper) so only the caller's uploads are
returned, and after calling updateFileComplete verify its return value indicates
a successful update (throw/abort if no document was modified) before proceeding.
Reference updateFileContentById, findByIds, updateFileComplete and
filesToConfirm when making these changes.
| const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => { | ||
| return new Promise((resolve) => { | ||
| const img = new Image(); | ||
| img.onload = () => { | ||
| resolve({ | ||
| height: img.height, | ||
| width: img.width, | ||
| }); | ||
| }; | ||
| img.src = dataURL; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Missing onerror handler — promise will hang on broken/invalid image data.
If the image fails to load (e.g., corrupted data, unsupported format), img.onload never fires and the promise never settles. Add a reject path via img.onerror.
Proposed fix
const getHeightAndWidthFromDataUrl = (dataURL: string): Promise<{ height: number; width: number }> => {
- return new Promise((resolve) => {
+ return new Promise((resolve, reject) => {
const img = new Image();
img.onload = () => {
resolve({
height: img.height,
width: img.width,
});
};
+ img.onerror = () => {
+ reject(new Error('Failed to load image'));
+ };
img.src = dataURL;
});
};🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts` around lines 16
- 27, The promise in getHeightAndWidthFromDataUrl can hang if the Image fails to
load; add an img.onerror handler that rejects the Promise with an Error (e.g.,
"Failed to load image") and ensure you attach error and load handlers before
setting img.src; also perform minimal cleanup inside both handlers (clear
onload/onerror) so resolve/reject only happens once and the promise never
remains pending on broken/invalid image data.
| ...(fileType === 'image' && { | ||
| image_dimensions: await getHeightAndWidthFromDataUrl(window.URL.createObjectURL(fileToUpload.file)), | ||
| }), |
There was a problem hiding this comment.
Object URL created but never revoked — memory leak.
window.URL.createObjectURL(fileToUpload.file) on Line 60 allocates a blob URL that persists until the page is unloaded or explicitly revoked. For multiple image uploads, this will accumulate leaked memory. Revoke the URL after dimensions are obtained.
🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts` around lines 59
- 61, The code creates a blob URL via
window.URL.createObjectURL(fileToUpload.file) for getting image dimensions but
never revokes it; update the call site (where getHeightAndWidthFromDataUrl is
invoked) to assign the blob URL to a variable, pass that variable into
getHeightAndWidthFromDataUrl, and ensure you call
window.URL.revokeObjectURL(blobUrl) after the dimensions are retrieved (use a
try/finally around the await to guarantee revoke even on error) so the blob URL
is always revoked and avoids memory leaks.
| async function continueSendingMessage() { | ||
| const fileUrls: string[] = []; | ||
| const filesToConfirm: IUploadToConfirm[] = []; | ||
|
|
||
| for await (const upload of filesToUpload) { | ||
| if (!upload.url || !upload.id) { | ||
| continue; | ||
| } | ||
|
|
||
| let content; | ||
| if (e2eRoom && isEncryptedUpload(upload)) { | ||
| content = await e2eRoom.encryptMessageContent(upload.metadataForEncryption); | ||
| } | ||
|
|
||
| fileUrls.push(upload.url); | ||
| filesToConfirm.push({ _id: upload.id, name: upload.file.name, content }); | ||
| } | ||
|
|
||
| const shouldConvertSentMessages = await e2eRoom?.shouldConvertSentMessages({ msg }); | ||
|
|
||
| let content; | ||
| if (e2eRoom && shouldConvertSentMessages) { | ||
| content = await getEncryptedContent(filesToUpload as EncryptedUpload[], e2eRoom, msg); | ||
| } |
There was a problem hiding this comment.
Encrypted content built from unfiltered uploads, but sending uses filtered uploads.
continueSendingMessage filters out uploads without url/id (Lines 159–162), but getEncryptedContent on Line 177 receives the unfiltered filesToUpload (cast to EncryptedUpload[]). This means the E2E-encrypted payload may reference files that were skipped during the send loop, causing a mismatch between content and filesToConfirm.
The encrypted content should be built from only the successfully uploaded files.
🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/processMessageUploads.ts` around lines 155
- 178, In continueSendingMessage the encryption step uses the original
filesToUpload (cast to EncryptedUpload[]) while the loop builds fileUrls and
filesToConfirm only from uploads that have url and id; filter filesToUpload the
same way before calling getEncryptedContent so the encrypted payload matches the
actually-sent files—e.g., create a filteredUploads (only uploads with upload.url
&& upload.id, and include metadataForEncryption when required) and pass that
filteredUploads to getEncryptedContent and any other encryption logic (keeping
references to continueSendingMessage, filesToUpload, fileUrls, filesToConfirm,
getEncryptedContent, EncryptedUpload, e2eRoom, shouldConvertSentMessages).
| const uploadFile = (file: File, encrypted?: EncryptedFileUploadContent) => { | ||
| if (encrypted) { | ||
| uploadsStore.send(file, encrypted); | ||
| } else { | ||
| uploadsStore.send(file); | ||
| } | ||
|
|
||
| uploadNextFile(); | ||
| }; |
There was a problem hiding this comment.
uploadsStore.send() is not awaited — queue processes all files in parallel, not sequentially.
uploadFile calls uploadsStore.send(file, encrypted) (returns Promise<void>) without await, then immediately calls uploadNextFile(). This means all files are dispatched to send nearly simultaneously. The queue/shift() pattern implies sequential intent. If parallel upload is desired, the queue abstraction is misleading; if sequential is desired, send should be awaited.
Proposed fix for sequential uploads
- const uploadFile = (file: File, encrypted?: EncryptedFileUploadContent) => {
+ const uploadFile = async (file: File, encrypted?: EncryptedFileUploadContent) => {
if (encrypted) {
- uploadsStore.send(file, encrypted);
+ await uploadsStore.send(file, encrypted);
} else {
- uploadsStore.send(file);
+ await uploadsStore.send(file);
}
uploadNextFile();
};🤖 Prompt for AI Agents
In `@apps/meteor/client/lib/chats/flows/uploadFiles.ts` around lines 23 - 31, The
uploadFile function dispatches uploadsStore.send(file, encrypted) without
awaiting it, causing all files to start in parallel; make uploadFile async and
await uploadsStore.send(...) before calling uploadNextFile() so the queue
processes uploads sequentially (update the uploadFile declaration and its
callers if needed to handle the async return). Ensure you reference uploadFile,
uploadsStore.send, and uploadNextFile when making the change so callers that
expect synchronous behavior are updated.
| const previewWrapperStyle = css` | ||
| background-color: 'surface-tint'; | ||
|
|
There was a problem hiding this comment.
background-color: 'surface-tint' is not a valid CSS value — it will be ignored by the browser.
Line 25 correctly resolves the token via Palette.surface['surface-hover'], but line 21 passes the raw string 'surface-tint' which is not a valid CSS color. This means the default (non-hover) background won't be applied.
🐛 Proposed fix
const previewWrapperStyle = css`
- background-color: 'surface-tint';
+ background-color: ${Palette.surface['surface-tint']};
&:hover {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const previewWrapperStyle = css` | |
| background-color: 'surface-tint'; | |
| const previewWrapperStyle = css` | |
| background-color: ${Palette.surface['surface-tint']}; | |
🤖 Prompt for AI Agents
In
`@apps/meteor/client/views/room/composer/messageBox/MessageComposerFileComponent.tsx`
around lines 20 - 22, The CSS value 'surface-tint' in the previewWrapperStyle is
an invalid literal string; replace the raw string with the resolved token value
(use Palette.surface['surface-tint'] like you already do for
Palette.surface['surface-hover']) so the non-hover background actually applies,
ensuring previewWrapperStyle references the Palette.surface token (and
import/namespace for Palette if missing).
| invalidContentType={Boolean(file.type && !fileUploadIsValidContentType(file.type))} | ||
| />, | ||
| ); | ||
| uploadFile(file); |
There was a problem hiding this comment.
Missing await on uploadFile(file) — fire-and-forget async call.
uploadFile is an async function that sets loading state and closes the modal in its finally block. Without await, handleUpload returns immediately after initiating the upload, and its caller cannot track the upload lifecycle. If the component unmounts before the upload completes, setIsLoading(false) and onClose() will attempt to update unmounted state.
Proposed fix
- uploadFile(file);
+ await uploadFile(file);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uploadFile(file); | |
| await uploadFile(file); |
🤖 Prompt for AI Agents
In
`@apps/meteor/client/views/room/webdav/WebdavFilePickerModal/WebdavFilePickerModal.tsx`
at line 147, handleUpload currently calls the async uploadFile(file) without
awaiting it; change handleUpload to be async and await uploadFile(file) so the
component waits for uploadFile's finally block (which calls setIsLoading(false)
and onClose()) before returning, preventing state updates on an unmounted
component and allowing callers to track the upload lifecycle; update any callers
or event handlers (e.g., button onClick) to handle the returned promise if
needed.
| await test.step('send text file again with blacklisted setting set, file upload should fail', async () => { | ||
| const composerFilesLocator = poHomeChannel.composer.getFileByName(TEST_FILE_TXT); | ||
| const composerFiles = await composerFilesLocator.all(); | ||
| await poHomeChannel.content.dragAndDropTxtFile(); | ||
| await poHomeChannel.content.descriptionInput.fill('message 3'); | ||
| await poHomeChannel.content.fileNameInput.fill('any_file3.txt'); | ||
| await poHomeChannel.content.btnModalConfirm.click(); | ||
|
|
||
| await Promise.all(composerFiles.map((file) => expect(file).toHaveAttribute('readonly'))); | ||
| await expect(poHomeChannel.content.lastUserMessage.locator('.rcx-icon--name-key')).toBeVisible(); | ||
| await expect(poHomeChannel.content.getFileDescription).toHaveText('message 2'); | ||
| await expect(poHomeChannel.content.lastMessageFileName).toContainText('any_file2.txt'); | ||
| }); |
There was a problem hiding this comment.
Assertion on pre-drag locator snapshot may be vacuous.
composerFilesLocator.all() on Line 120 resolves the locator before dragAndDropTxtFile() is called on Line 121. If no files matching TEST_FILE_TXT exist in the composer at that point, composerFiles is an empty array and Promise.all([]) on Line 123 resolves immediately — making no assertions at all. The readonly check would silently pass without verifying anything.
Consider resolving .all() after the drag-and-drop, or assert a minimum expected count before checking attributes.
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/e2e-encryption/e2ee-file-encryption.spec.ts` around
lines 118 - 125, The assertion is vacuous because composerFilesLocator.all()
(composerFiles) is resolved before calling
poHomeChannel.content.dragAndDropTxtFile(), so if no matching files exist it
returns an empty array; fix by calling composerFilesLocator.all() after
poHomeChannel.content.dragAndDropTxtFile() (or assert composerFiles.length > 0
immediately after resolving) and then run Promise.all(...) to check the readonly
attribute on each file; also ensure the test still checks
poHomeChannel.content.lastUserMessage.locator('.rcx-icon--name-key') afterwards.
| await poHomeChannel.content.inputFileUploadName.fill(updatedFileName); | ||
| await poHomeChannel.content.btnUpdateFileUpload.click(); | ||
|
|
||
| expect(poHomeChannel.composer.getFileByName(updatedFileName)); |
There was a problem hiding this comment.
expect() without a matcher is a no-op — this doesn't assert anything.
expect(poHomeChannel.composer.getFileByName(updatedFileName)) just creates an ExpectationObject and discards it. The same issue occurs on lines 63, 64, 118, and 119.
Proposed fix (apply similar changes to lines 63, 64, 118, 119)
- expect(poHomeChannel.composer.getFileByName(updatedFileName));
+ await expect(poHomeChannel.composer.getFileByName(updatedFileName)).toBeVisible();As per coding guidelines: "Use expect matchers for assertions (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements in Playwright tests" and "Prefer web-first assertions (toBeVisible, toHaveText, etc.) in Playwright tests."
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/file-upload.spec.ts` at line 52, The test currently
calls expect(poHomeChannel.composer.getFileByName(updatedFileName)) with no
matcher, which is a no-op; update each such call (including
poHomeChannel.composer.getFileByName(updatedFileName) and the other occurrences
on lines 63, 64, 118, 119) to use an appropriate Playwright matcher (e.g.,
append .toBeTruthy(), .toBeDefined(), or a UI-focused assertion like
.toBeVisible()/toHaveText() depending on whether getFileByName returns an
element or a data value) so the expectation actually asserts the intended
condition; locate the calls via poHomeChannel.composer.getFileByName and replace
them with expect(...).<chosenMatcher>().
| test('should respect the maximum number of files allowed per message: 10', async () => { | ||
| const file11 = 'number6.png'; | ||
| const files = new Array(10).fill('number1.png'); | ||
|
|
||
| await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file))); | ||
| await poHomeChannel.content.dragAndDropTxtFile(); | ||
|
|
||
| await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10); | ||
| await expect(poHomeChannel.composer.getFileByName(file11)).not.toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
file11 variable is misleading and its assertion is vacuous.
file11 is set to 'number6.png' but no file with that name is ever uploaded. The assertion on line 152 (getFileByName(file11)).not.toBeVisible()) will always pass trivially. The intent appears to be verifying that the 11th file (TEST_FILE_TXT from the drag-and-drop on line 149) was rejected due to the 10-file limit.
Proposed fix
test('should respect the maximum number of files allowed per message: 10', async () => {
- const file11 = 'number6.png';
const files = new Array(10).fill('number1.png');
await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file)));
await poHomeChannel.content.dragAndDropTxtFile();
await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10);
- await expect(poHomeChannel.composer.getFileByName(file11)).not.toBeVisible();
+ await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).not.toBeVisible();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('should respect the maximum number of files allowed per message: 10', async () => { | |
| const file11 = 'number6.png'; | |
| const files = new Array(10).fill('number1.png'); | |
| await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file))); | |
| await poHomeChannel.content.dragAndDropTxtFile(); | |
| await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10); | |
| await expect(poHomeChannel.composer.getFileByName(file11)).not.toBeVisible(); | |
| }); | |
| test('should respect the maximum number of files allowed per message: 10', async () => { | |
| const files = new Array(10).fill('number1.png'); | |
| await Promise.all(files.map((file) => poHomeChannel.content.sendFileMessage(file))); | |
| await poHomeChannel.content.dragAndDropTxtFile(); | |
| await expect(poHomeChannel.composer.getFilesInComposer()).toHaveCount(10); | |
| await expect(poHomeChannel.composer.getFileByName(TEST_FILE_TXT)).not.toBeVisible(); | |
| }); |
🤖 Prompt for AI Agents
In `@apps/meteor/tests/e2e/file-upload.spec.ts` around lines 144 - 153, The test
uses a misleading file11 ('number6.png') and thus the not.toBeVisible()
assertion is vacuous; update the test in the "should respect the maximum number
of files allowed per message: 10" case to reference the actual 11th filename
produced by poHomeChannel.content.dragAndDropTxtFile() (or use the TEST_FILE_TXT
constant used by that helper) instead of 'number6.png', and assert that
poHomeChannel.composer.getFileByName(<actual-11th-filename>) is not visible (and
keep the getFilesInComposer count assertion) so the test verifies the dropped
TXT file was rejected due to the 10-file limit.
| ]), | ||
| ); | ||
|
|
||
| if (filesToConfirm && filesToConfirm.length > MAX_MULTIPLE_UPLOADED_FILES) { |
There was a problem hiding this comment.
| if (filesToConfirm && filesToConfirm.length > MAX_MULTIPLE_UPLOADED_FILES) { | |
| if (filesToConfirm?.length > MAX_MULTIPLE_UPLOADED_FILES) { |
| }); | ||
| } | ||
|
|
||
| return continueSendingMessage(); |
There was a problem hiding this comment.
nitpick: maybe an iife? seems a bit weird to return a function that's defined on next line next to the return 🤔
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
kodiakhq
bot
removed
the
stat: ready to merge
13 participants
Proposed changes (including videos or screenshots)
This PR aims to introduce the ability to send multiple files into a single message. In order to keep the compatibility with old integrations we're introducing a new setting, in order to use it you'll need to enabled it first otherwise it will send one file per message
Note
It's not available for livechat rooms yet
It also changes how're handle uploads, for now if you upload multiple files they will be attached in the composer for better visibility
Closes #35651
Closes #24776
Closes #25918
Closes RocketChat/feature-requests#31
Steps to test or reproduce
FileUpload_EnableMultipleFilesPerMessageFurther comments
This implementation is an initial version of the multiple file sharing feature. While it is functional, some minor bugs remain, particularly with image file sharing, which will be addressed in future updates.
CORE-847
This pull request introduces the ability to send multiple files in a single message within the Rocket.Chat application. Key changes include:
File Upload Enhancements: The file upload functionality has been significantly refactored to support multiple files, including updates to the UploadsAPI interface with new methods for managing uploads and handling encrypted content. The Upload type definition now includes both file and URL properties.
New Components and Hooks: Several new components and hooks have been added to manage file uploads, such as
MessageComposerFile,MessageComposerFileArea, andMessageComposerFileLoader, which provide UI elements for file management, preview, and error handling. TheuseFileUploadDropTargethook has been introduced to streamline file upload handling.Thread and Message Integration: The PR adds support for thread-specific uploads and integrates file upload capabilities into various components like
MessageBox,AudioMessageRecorder, andVideoMessageRecorder. TheconfirmFilesfunction is introduced to handle file confirmation and sending, with error handling and cleanup processes.Localization and API Updates: The English localization file has been updated with new messages related to upload failures and file count limitations. A new API endpoint for media editing has been added to the REST typings.
Overall, this pull request enhances the file upload experience by allowing multiple files to be sent in a single message, improving the user interface, and refining the underlying upload logic.
Summary by CodeRabbit
New Features
Improvements