feat: custom sounds create/update API endpoints with file validations#39617
Open
nazabucciarelli wants to merge 34 commits intodevelopfrom
Open
feat: custom sounds create/update API endpoints with file validations#39617nazabucciarelli wants to merge 34 commits intodevelopfrom
nazabucciarelli wants to merge 34 commits intodevelopfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 17633dd The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds authenticated REST endpoints for creating/updating custom sounds with multipart parsing, server/client MIME and 5MB size validation, new upload/persistence helpers, model updates, client UI/hooks adjustments to FormData uploads, and expanded end-to-end tests and typings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as "API Endpoint"
participant Parser as "getUploadFormData"
participant Validator as "AJV + FileValidation"
participant Persist as "insertOrUpdateSound"
participant Storage as "uploadCustomSound"
participant Model as "CustomSoundsModel"
participant Broadcaster as "notify/api.broadcast"
Client->>API: POST /v1/custom-sounds.create (FormData: file, name, extension)
API->>Parser: parse multipart → buffer + fields
Parser->>Validator: validate body (AJV) and file (MIME + size)
Validator-->>API: ok / throw (formatted errors)
API->>Persist: insertOrUpdateSound({name, extension, _id?})
Persist->>Model: find/insert/update document (uniqueness, setName/setExtension)
Persist-->>API: returns _id
API->>Storage: uploadCustomSound(buffer, contentType, { _id, extension, previousExtension? })
Storage->>Storage: delete previous file? → write new file (stream)
Storage->>Broadcaster: notify.updateCustomSound (after write)
API->>Client: success response (new _id / {})
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39617 +/- ##
===========================================
- Coverage 70.61% 70.60% -0.01%
===========================================
Files 3257 3257
Lines 115789 115796 +7
Branches 21022 20994 -28
===========================================
+ Hits 81759 81760 +1
- Misses 31968 31975 +7
+ Partials 2062 2061 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
nazabucciarelli
force-pushed
the
feat/custom-sound-create-endpoint
branch
from
6116940 to
9c46b02
Compare
…Chat/Rocket.Chat into feat/custom-sound-create-endpoint
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
coderabbitai
bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
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/hooks/useSingleFileInput.ts (1)
4-10:⚠️ Potential issue | 🔴 CriticalFix type mismatch in
UserAvatarEditorcallback.The
useSingleFileInputhook now passes two arguments(file: File, formData: FormData)to the callback, butUserAvatarEditor.setUploadedPreviewexpects(file: File, avatarObj: AvatarObject). TheFormDataobject passed as the second argument is incompatible with the expectedAvatarObjecttype, causing a type error.Other callers (
handleChangeFile,setEmojiFile,setEmojiPreview,handleChangeAvatar) work correctly as they only accept a single parameter and ignore the extra argument.Update
UserAvatarEditorto handle theFormDataparameter or change the hook to maintain backward compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/hooks/useSingleFileInput.ts` around lines 4 - 10, The hook useSingleFileInput currently invokes onSetFile(file, formData) which breaks UserAvatarEditor.setUploadedPreview because it expects (file: File, avatarObj: AvatarObject); fix by making useSingleFileInput only call onSetFile(file) to preserve existing single-argument callers, and either (a) add a new optional callback parameter (e.g., onSetFileWithFormData) for consumers that need the FormData, or (b) change the onSetFile second-argument type to a union (AvatarObject | FormData) and update UserAvatarEditor.setUploadedPreview to accept FormData by converting it into an AvatarObject; update the relevant references: useSingleFileInput and UserAvatarEditor.setUploadedPreview to keep types consistent and avoid passing raw FormData where AvatarObject is expected.apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts (1)
17-23:⚠️ Potential issue | 🔴 CriticalRetire or harden this DDP upload path too.
The new REST endpoints add multipart parsing and upload guards, but this Meteor method still accepts arbitrary binary content and caller-supplied
contentType, then writes it straight to storage. As long as this remains callable, the arbitrary-upload fix is bypassable, and the full payload is still materialized in memory without a server-side size cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts` around lines 17 - 23, The Meteor DDP method uploadCustomSound currently accepts raw binaryContent and caller-supplied contentType and writes it straight to storage, bypassing REST guards and materializing the full payload in memory; either retire this DDP method or harden it: remove/expose the method so clients must use the REST multipart endpoint, or add strict server-side checks in the Meteor method uploadCustomSound — validate contentType against a whitelist (e.g., 'audio/mpeg','audio/wav'), enforce a max size before Buffer.from by checking binaryContent.length and reject oversized payloads, and avoid fully buffering by streaming to a temp file (use fs.createWriteStream) and aborting if size limit exceeded, then call the existing uploadCustomSound storage helper only after validation/streaming succeeds.
🧹 Nitpick comments (2)
apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts (2)
17-24: Remove code comments per coding guidelines.As per coding guidelines for
**/*.{ts,tsx,js}: avoid code comments in the implementation.🧹 Proposed fix to remove comments
- // let nameValidation = new RegExp('^[0-9a-zA-Z-_+;.]+$'); - - // allow all characters except colon, whitespace, comma, >, <, &, ", ', /, \, (, ) - // more practical than allowing specific sets of characters; also allows foreign languages const nameValidation = /[\s,:><&"'\/\\\(\)]/; - // silently strip colon; this allows for uploading :soundname: as soundname soundData.name = soundData.name.replace(/:/g, '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts` around lines 17 - 24, Remove the inline comments above and around the regex and replace statement: delete the commented-out RegExp line, the explanatory comment block before the nameValidation declaration, and the inline "silently strip colon" comment next to soundData.name.replace; keep the const nameValidation = /[\s,:><&"'\/\\\(\)]/; and the soundData.name = soundData.name.replace(/:/g, ''); lines as-is (or optionally rename nameValidation to something self-descriptive like invalidNameChars to retain clarity) so no implementation comments remain.
63-83: Consider consolidating duplicate broadcasts.When both
nameandextensionchange, two identicalnotify.updateCustomSoundbroadcasts are emitted with the same payload. This could be consolidated into a single broadcast after all updates complete.♻️ Proposed refactor to consolidate broadcasts
+ let shouldBroadcast = false; + if (soundData.name !== soundData.previousName) { await CustomSounds.setName(soundData._id, soundData.name); - void api.broadcast('notify.updateCustomSound', { - soundData: { - _id: soundData._id, - name: soundData.name, - extension: soundData.extension, - }, - }); + shouldBroadcast = true; } if (soundData.extension !== soundData.previousExtension) { await CustomSounds.setExtension(soundData._id, soundData.extension); - void api.broadcast('notify.updateCustomSound', { - soundData: { - _id: soundData._id, - name: soundData.name, - extension: soundData.extension, - }, - }); + shouldBroadcast = true; } + + if (shouldBroadcast) { + void api.broadcast('notify.updateCustomSound', { + soundData: { + _id: soundData._id, + name: soundData.name, + extension: soundData.extension, + }, + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts` around lines 63 - 83, Currently code calls CustomSounds.setName and/or CustomSounds.setExtension and emits api.broadcast('notify.updateCustomSound', ...) twice when both name and extension changed; change the logic in insertOrUpdateSound to track whether nameChanged (soundData.name !== soundData.previousName) and extensionChanged (soundData.extension !== soundData.previousExtension), perform the awaited updates via CustomSounds.setName(soundData._id, ...) and CustomSounds.setExtension(soundData._id, ...) as needed, and after both updates complete emit a single api.broadcast('notify.updateCustomSound', { soundData: { _id: soundData._id, name: soundData.name, extension: soundData.extension } }) if either change occurred so the payload is consolidated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/custom-sounds.ts`:
- Around line 219-254: The route currently passes fields.extension to
insertOrUpdateSound/uploadCustomSound even when no file was uploaded, causing
rename-only saves to send undefined; update the logic around insertOrUpdateSound
and uploadCustomSound calls (located near getUploadFormData,
CustomSounds.findOneById, insertOrUpdateSound, and uploadCustomSound) to use
soundToUpdate.extension as the fallback whenever fileBuffer is falsy — i.e.,
compute an effectiveExtension = fileBuffer ? fields.extension :
soundToUpdate.extension and pass effectiveExtension (and previousExtension still
from soundToUpdate.extension) into insertOrUpdateSound and uploadCustomSound
accordingly.
- Around line 168-189: Validate the uploaded bytes (fileBuffer) for an actual
audio signature before any DB or storage writes: add a shared detector function
(e.g., detectMimeFromBuffer or isAllowedAudioBuffer) and call it immediately
after extracting { fields, fileBuffer, mimetype } from getUploadFormData; if
detection fails or the detected MIME/signature is not in the allowed audio set,
throw/reject and skip calling insertOrUpdateSound or uploadCustomSound. Apply
the same pre-write check to the other handler block that also gets fileBuffer
(the second occurrence covering lines 219-254) so both flows validate decoded
bytes rather than relying solely on form metadata (fields.extension or request
MIME).
- Around line 183-189: The code currently calls insertOrUpdateSound before
uploadCustomSound, which can leave orphan DB records or inconsistent metadata if
the file write fails; change the flow to either stage/write the file first and
only call insertOrUpdateSound after a successful upload, or keep the DB write
but add compensation in the catch block to delete or rollback the
inserted/updated record (use the returned _id from insertOrUpdateSound to remove
it on failure). Update both the create flow around
insertOrUpdateSound/uploadCustomSound and the update flow (the similar code at
the 240-254 section) to follow the same pattern so DB state and disk state
remain consistent.
In `@apps/meteor/app/custom-sounds/server/lib/uploadCustomSound.ts`:
- Around line 13-23: The upload currently deletes the previous file
unconditionally and resolves on the writable stream's 'end' without handling
errors; update the logic in uploadCustomSound to (1) only attempt deleteFile
when soundData.previousExtension is truthy, (2) create the write stream via
RocketChatFileCustomSoundsInstance.createWriteStream and attach 'finish' and
'error' handlers on the writable (ws) and 'error' on the readable from
RocketChatFile.bufferToStream(rs), (3) defer deleting the previous file until
the new write finishes successfully (i.e., inside ws 'finish'), and (4) make the
Promise reject on any stream 'error' to avoid hanging; keep the existing
setTimeout broadcast('notify.updateCustomSound', { soundData }) after successful
finish.
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 128-129: The UI in EditSound still shows the persisted sound name
(data.name) after calling setFile(soundFile), preventing users from seeing the
newly selected replacement; update the display logic in EditSound to prefer the
local state file name (the variable set by setFile / soundFile) when present,
falling back to the translated 'none' string (same translation key/behavior used
in the create form) and otherwise to data.name; adjust the JSX around
IconButton/clickUpload to render soundFile.name (or the equivalent local state
getter) first, then data.name, and ensure the fallback uses the app's i18n
translation helper as in the Create form.
In `@apps/meteor/client/views/admin/customSounds/lib.ts`:
- Around line 41-42: The extension extraction using
soundFile?.name?.split('.').pop() is unsafe for dotless names and preserves
arbitrary casing; update the logic that sets the extension (the extension
variable derived from soundFile?.name and the similar code around lines handling
newFile) to: check for the last '.' with lastIndexOf, ensure it's present and
not the final character, extract the substring after that, normalize to
lowercase and strip any leading/trailing dots, and fall back to an empty string
or a validated MIME->ext mapping if the result is invalid; apply the same
normalized extraction wherever extension is computed so the API always receives
a clean, lowercase known extension or ''.
- Around line 17-18: The current MIME check using
soundFile.type.startsWith('audio/') is too permissive; replace it with an
explicit allowlist of safe MIME types (e.g., 'audio/mpeg', 'audio/wav',
'audio/ogg', etc.) and validate soundFile.type against that list in the same
location where soundFile and errors are handled, so only allowed types are
accepted; additionally, mirror this allowlist/validation on the server side (see
uploadCustomSound and custom-sounds API handlers) and optionally add a secondary
check using file extension/codec if available to harden acceptance.
In `@packages/rest-typings/src/v1/customSounds.ts`:
- Around line 74-96: The schema and type disagree: CustomSoundsUpdate currently
requires _id and name in CustomSoundsUpdateSchema but the TypeScript type
CustomSoundsUpdate includes extension as required; update either the schema or
the type and the endpoint usage—either add 'extension' to
CustomSoundsUpdateSchema.required (so isCustomSoundsUpdateProps enforces
extension) or change the TypeScript alias CustomSoundsUpdate to make extension
optional (e.g., pick only '_id'|'name' and make extension?: string) and update
the server handler that reads fields.extension to safely handle undefined
(apps/meteor/app/api/server/v1/custom-sounds.ts handler that reads
fields.extension). Ensure isCustomSoundsUpdateProps, CustomSoundsUpdateSchema,
and the endpoint logic remain consistent.
---
Outside diff comments:
In `@apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts`:
- Around line 17-23: The Meteor DDP method uploadCustomSound currently accepts
raw binaryContent and caller-supplied contentType and writes it straight to
storage, bypassing REST guards and materializing the full payload in memory;
either retire this DDP method or harden it: remove/expose the method so clients
must use the REST multipart endpoint, or add strict server-side checks in the
Meteor method uploadCustomSound — validate contentType against a whitelist
(e.g., 'audio/mpeg','audio/wav'), enforce a max size before Buffer.from by
checking binaryContent.length and reject oversized payloads, and avoid fully
buffering by streaming to a temp file (use fs.createWriteStream) and aborting if
size limit exceeded, then call the existing uploadCustomSound storage helper
only after validation/streaming succeeds.
In `@apps/meteor/client/hooks/useSingleFileInput.ts`:
- Around line 4-10: The hook useSingleFileInput currently invokes
onSetFile(file, formData) which breaks UserAvatarEditor.setUploadedPreview
because it expects (file: File, avatarObj: AvatarObject); fix by making
useSingleFileInput only call onSetFile(file) to preserve existing
single-argument callers, and either (a) add a new optional callback parameter
(e.g., onSetFileWithFormData) for consumers that need the FormData, or (b)
change the onSetFile second-argument type to a union (AvatarObject | FormData)
and update UserAvatarEditor.setUploadedPreview to accept FormData by converting
it into an AvatarObject; update the relevant references: useSingleFileInput and
UserAvatarEditor.setUploadedPreview to keep types consistent and avoid passing
raw FormData where AvatarObject is expected.
---
Nitpick comments:
In `@apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts`:
- Around line 17-24: Remove the inline comments above and around the regex and
replace statement: delete the commented-out RegExp line, the explanatory comment
block before the nameValidation declaration, and the inline "silently strip
colon" comment next to soundData.name.replace; keep the const nameValidation =
/[\s,:><&"'\/\\\(\)]/; and the soundData.name = soundData.name.replace(/:/g,
''); lines as-is (or optionally rename nameValidation to something
self-descriptive like invalidNameChars to retain clarity) so no implementation
comments remain.
- Around line 63-83: Currently code calls CustomSounds.setName and/or
CustomSounds.setExtension and emits api.broadcast('notify.updateCustomSound',
...) twice when both name and extension changed; change the logic in
insertOrUpdateSound to track whether nameChanged (soundData.name !==
soundData.previousName) and extensionChanged (soundData.extension !==
soundData.previousExtension), perform the awaited updates via
CustomSounds.setName(soundData._id, ...) and
CustomSounds.setExtension(soundData._id, ...) as needed, and after both updates
complete emit a single api.broadcast('notify.updateCustomSound', { soundData: {
_id: soundData._id, name: soundData.name, extension: soundData.extension } }) if
either change occurred so the payload is consolidated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f27aacf-8444-4635-9f93-f76856504328
📒 Files selected for processing (15)
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/CustomSoundsPage.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/lib/constants.tspackages/model-typings/src/models/ICustomSoundsModel.tspackages/models/src/models/CustomSounds.tspackages/rest-typings/src/v1/customSounds.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/models/CustomSounds.tsapps/meteor/lib/constants.tsapps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tsapps/meteor/client/views/admin/customSounds/CustomSoundsPage.tsxpackages/rest-typings/src/v1/customSounds.tsapps/meteor/app/api/server/lib/getUploadFormData.tspackages/model-typings/src/models/ICustomSoundsModel.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
🧠 Learnings (19)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/models/src/models/CustomSounds.tsapps/meteor/lib/constants.tsapps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tspackages/rest-typings/src/v1/customSounds.tsapps/meteor/app/api/server/lib/getUploadFormData.tspackages/model-typings/src/models/ICustomSoundsModel.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/models/src/models/CustomSounds.tsapps/meteor/lib/constants.tsapps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tspackages/rest-typings/src/v1/customSounds.tsapps/meteor/app/api/server/lib/getUploadFormData.tspackages/model-typings/src/models/ICustomSoundsModel.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.tsapps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/client/views/admin/customSounds/lib.ts
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
packages/rest-typings/src/v1/customSounds.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
packages/rest-typings/src/v1/customSounds.tsapps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts
🔇 Additional comments (7)
apps/meteor/client/views/admin/customSounds/lib.ts (1)
4-5: Good move to use the nativeFiletype.Using
Fileinvalidate/createSoundDataremoves custom type drift and matches browser upload semantics cleanly.Also applies to: 27-27
apps/meteor/lib/constants.ts (1)
2-2: LGTM!The constant centralizes the max file size limit for custom sounds, enabling consistent enforcement across both server-side validation and client-side UI components.
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
105-105: LGTM!Including
instancePathin validation errors improves debuggability by clearly indicating which field failed validation.packages/model-typings/src/models/ICustomSoundsModel.ts (1)
11-11: LGTM!The new
setExtensionmethod follows the established pattern of the existingsetNamemethod, maintaining consistency in the interface.packages/models/src/models/CustomSounds.ts (1)
49-58: LGTM!The
setExtensionimplementation correctly mirrors the existingsetNamepattern, using the same MongoDB update approach.apps/meteor/client/hooks/useSingleFileInput.ts (1)
45-52: Size validation logic is correctly implemented.The guard properly checks the file size against the optional
maxSize, invokesonErrorif provided, clears the input to allow retry, and returns early to prevent further processing.apps/meteor/app/custom-sounds/server/methods/insertOrUpdateSound.ts (1)
27-33: LGTM!Clean separation of concerns: the Meteor method correctly performs permission checks before delegating to the helper function for business logic.
nazabucciarelli
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/lib/getUploadFormData.ts">
<violation number="1" location="apps/meteor/app/api/server/lib/getUploadFormData.ts:105">
P1: This can throw while building the validation message because `.join()` is called on a potentially undefined result from `errors?.map(...)`.</violation>
</file>
<file name="apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx">
<violation number="1" location="apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx:56">
P2: Avoid throwing inside handleSave without handling the error. The validation error and failed upload will create unhandled promise rejections because there is no try/catch around this async handler.</violation>
</file>
<file name="apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts">
<violation number="1" location="apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts:24">
P2: Sanitizing `:` after the required-field check allows empty sound names (for values like `":"`) to slip through. Re-validate required name after sanitization.</violation>
</file>
<file name="apps/meteor/client/views/admin/customSounds/EditSound.tsx">
<violation number="1" location="apps/meteor/client/views/admin/customSounds/EditSound.tsx:62">
P2: Avoid throwing inside the validation loop; it can surface as an unhandled rejection after showing the toast. Return early once validation fails instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/custom-sounds.ts (1)
181-183:⚠️ Potential issue | 🔴 CriticalValidate the uploaded bytes, not the multipart
mimetype.
mimetypeis still client-controlled metadata. A renamed non-audio payload can pass both checks if the caller sends an allowedContent-Type. The handlers already havefileBuffer, so detect the real type from those bytes and derive the stored extension from that result before callinginsertOrUpdateSound()/uploadCustomSound().Also applies to: 237-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-sounds.ts` around lines 181 - 183, The current checks use the client-supplied mimetype which is untrusted; instead, inspect the uploaded bytes (fileBuffer) to determine the actual MIME/type and extension, validate that detected type is in CUSTOM_SOUND_ALLOWED_MIME_TYPES, and derive the extension from that detection before calling insertOrUpdateSound() or uploadCustomSound(); update the validation at the blocks around the mimetype check (including the duplicate at lines noted) to replace reliance on mimetype with a byte-based detection step (use the existing fileBuffer variable and then pass the detected extension/type to insertOrUpdateSound() / uploadCustomSound()).
🧹 Nitpick comments (2)
apps/meteor/client/hooks/useEndpointUploadMutation.ts (2)
5-9: TightenTDataand avoidanyin the upload result contract.
[key: string]: anyand unconstrainedTDatamakeresult as TDatatoo permissive. Useunknownand constrainTDatatoIUploadResultto keeponSuccesspayload typing safer across callers.Suggested refactor
interface IUploadResult { success: boolean; status?: string; - [key: string]: any; + [key: string]: unknown; } -type UseEndpointUploadOptions<TData> = Omit<UseMutationOptions<TData, Error, FormData>, 'mutationFn'>; +type UseEndpointUploadOptions<TData extends IUploadResult> = Omit<UseMutationOptions<TData, Error, FormData>, 'mutationFn'>; -export const useEndpointUploadMutation = <TPathPattern extends PathFor<'POST'>, TData = IUploadResult>( +export const useEndpointUploadMutation = <TPathPattern extends PathFor<'POST'>, TData extends IUploadResult = IUploadResult>( endpoint: TPathPattern, options?: UseEndpointUploadOptions<TData>, ) => { ... - const result = await promise; + const result = (await promise) as IUploadResult;As per coding guidelines,
**/*.{ts,tsx,js}: "Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests."Also applies to: 11-16, 21-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/hooks/useEndpointUploadMutation.ts` around lines 5 - 9, Tighten the upload result typing by replacing the open index signature and any usage with unknown and constraining the generic: change IUploadResult to use [key: string]: unknown (or remove the index signature if not needed) and update the hook's generic from TData to TData extends IUploadResult so callers get a properly constrained payload; update any places in useEndpointUploadMutation (including where result is cast with "as TData") to return/propagate the typed IUploadResult instead of any, and adjust onSuccess handler signatures to accept TData extends IUploadResult to keep payload typing safe.
13-17: Narrow the endpoint generic to POST and remove the cast.
TPathPattern extends PathPatternplusas PathFor<'POST'>sidesteps compile-time method checks. Type the generic directly asPathFor<'POST'>so invalid non-POST endpoints fail at compile time.Suggested refactor
-import type { PathFor, PathPattern } from '@rocket.chat/rest-typings'; +import type { PathFor } from '@rocket.chat/rest-typings'; ... -export const useEndpointUploadMutation = <TPathPattern extends PathPattern, TData = IUploadResult>( +export const useEndpointUploadMutation = <TPathPattern extends PathFor<'POST'>, TData = IUploadResult>( endpoint: TPathPattern, options?: UseEndpointUploadOptions<TData>, ) => { - const sendData = useUpload(endpoint as PathFor<'POST'>); + const sendData = useUpload(endpoint);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/hooks/useEndpointUploadMutation.ts` around lines 13 - 17, The generic for useEndpointUploadMutation is too broad and then cast to PathFor<'POST'>; change the generic parameter TPathPattern from extending PathPattern to extending PathFor<'POST'> in the useEndpointUploadMutation signature so invalid non-POST endpoints fail at compile time, and remove the runtime cast ("as PathFor<'POST'>") when calling useUpload; ensure references to IUploadResult and UseEndpointUploadOptions stay the same and that useUpload(endpoint) now accepts the typed endpoint without casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/admin/customSounds/lib.ts`:
- Around line 60-63: The update branch is using the raw name string so
whitespace-only renames pass validate(); trim the name like the create branch
(apply name = name.trim()) before running validate() and before returning the
update object (the one that references previousData._id, name and
getExtension(soundFile)) so validate() sees the normalized value; ensure any
subsequent use of name in the update payload uses the trimmed value.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/custom-sounds.ts`:
- Around line 181-183: The current checks use the client-supplied mimetype which
is untrusted; instead, inspect the uploaded bytes (fileBuffer) to determine the
actual MIME/type and extension, validate that detected type is in
CUSTOM_SOUND_ALLOWED_MIME_TYPES, and derive the extension from that detection
before calling insertOrUpdateSound() or uploadCustomSound(); update the
validation at the blocks around the mimetype check (including the duplicate at
lines noted) to replace reliance on mimetype with a byte-based detection step
(use the existing fileBuffer variable and then pass the detected extension/type
to insertOrUpdateSound() / uploadCustomSound()).
---
Nitpick comments:
In `@apps/meteor/client/hooks/useEndpointUploadMutation.ts`:
- Around line 5-9: Tighten the upload result typing by replacing the open index
signature and any usage with unknown and constraining the generic: change
IUploadResult to use [key: string]: unknown (or remove the index signature if
not needed) and update the hook's generic from TData to TData extends
IUploadResult so callers get a properly constrained payload; update any places
in useEndpointUploadMutation (including where result is cast with "as TData") to
return/propagate the typed IUploadResult instead of any, and adjust onSuccess
handler signatures to accept TData extends IUploadResult to keep payload typing
safe.
- Around line 13-17: The generic for useEndpointUploadMutation is too broad and
then cast to PathFor<'POST'>; change the generic parameter TPathPattern from
extending PathPattern to extending PathFor<'POST'> in the
useEndpointUploadMutation signature so invalid non-POST endpoints fail at
compile time, and remove the runtime cast ("as PathFor<'POST'>") when calling
useUpload; ensure references to IUploadResult and UseEndpointUploadOptions stay
the same and that useUpload(endpoint) now accepts the typed endpoint without
casting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4fda4d2-44dd-4af0-a5dc-5f0f91904b78
📒 Files selected for processing (8)
.changeset/mighty-icons-kiss.mdapps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/lib/constants.tspackages/rest-typings/src/v1/customSounds.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/mighty-icons-kiss.md
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/lib/constants.ts
- packages/rest-typings/src/v1/customSounds.ts
- apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
- apps/meteor/client/views/admin/customSounds/EditSound.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
🧠 Learnings (28)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/client/hooks/useEndpointUploadMutation.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/lib.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/hooks/useEndpointUploadMutation.tsapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/custom-sounds.ts
🔇 Additional comments (2)
apps/meteor/client/hooks/useEndpointUploadMutation.ts (1)
21-29: Good change: returning mutation data while preserving failure flow.Returning
resultfrommutationFnenables endpoint-specificonSuccesshandling (e.g., custom sounds) without changing the existing error-toast behavior.apps/meteor/app/api/server/v1/custom-sounds.ts (1)
188-192: No fix needed. ThedeleteFileimplementation in GridFS (apps/meteor/app/file/server/file.server.ts:120-123) explicitly handles missing files as a no-op by checkingif (file == null) { return undefined; }. WhenuploadCustomSoundattempts to delete${_id}.undefinedon first-time uploads (sincepreviousExtensionis undefined), the deletion simply returns early without error, allowing the new file to be written immediately after.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
27-33: Mutation success callback accessessound._idwithout null-check.The
onSuccesscallback destructures{ sound }from the response and callsgoToNew(sound._id)(). If the API response structure changes or returns an unexpected shape, this could throw at runtime.Consider adding defensive handling or ensuring the response type is properly typed to guarantee
sound._idis always present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx` around lines 27 - 33, The onSuccess callback for useEndpointUploadMutation currently assumes the response includes sound._id and calls goToNew(sound._id)(); make this defensive: verify that the destructured sound exists and that sound._id is defined before invoking goToNew; if missing, still call dispatchToastMessage and onChange but skip or handle navigation (e.g., log/warn or use a fallback) to avoid runtime exceptions in saveAction/onSuccess.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 41-46: The onSuccess callback for useEndpointUploadMutation
(saveAction) currently calls dispatchToastMessage and close(), but handleSave
calls onChange() after await saveAction(formData) which may run after close()
unmounts the component; move the call to onChange() into the onSuccess callback
(call onChange() before close()) inside the useEndpointUploadMutation config,
and remove the unconditional onChange() call from handleSave so the change
handler runs only while the component is still mounted; update references to
saveAction, onSuccess, dispatchToastMessage, close, handleSave, and onChange
accordingly.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx`:
- Around line 27-33: The onSuccess callback for useEndpointUploadMutation
currently assumes the response includes sound._id and calls
goToNew(sound._id)(); make this defensive: verify that the destructured sound
exists and that sound._id is defined before invoking goToNew; if missing, still
call dispatchToastMessage and onChange but skip or handle navigation (e.g.,
log/warn or use a fallback) to avoid runtime exceptions in saveAction/onSuccess.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 870936c5-fcb9-4d27-9513-82db011eaa3b
📒 Files selected for processing (5)
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts
- apps/meteor/client/views/admin/customSounds/lib.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
🧠 Learnings (19)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/lib/getUploadFormData.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
🔇 Additional comments (7)
apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
104-108: LGTM - improved error formatting for AJV validation failures.The change provides clearer error messages by including the
instancePathalongside each validation message. The?? []fallback correctly handles cases whereerrorsis null or undefined.Minor optional refinement:
e.messageis typed asstring | undefinedin AJV. While AJV typically always provides messages for built-in validators, you could add a fallback for robustness:♻️ Optional: Handle potential undefined message
if (options.validate !== undefined && !options.validate(fields)) { return reject( - new MeteorError(`Invalid fields ${(options.validate.errors ?? []).map((e) => `${e.instancePath} ${e.message}`).join(', ')}`), + new MeteorError(`Invalid fields ${(options.validate.errors ?? []).map((e) => `${e.instancePath} ${e.message ?? 'invalid'}`).join(', ')}`), ); }[approve_code_changes, suggest_optional_refactor]
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (2)
52-72: LGTM – handleSave validation and FormData construction.The validation flow correctly checks for required fields, shows a localized error toast for the first invalid field, and returns early. The FormData assembly conditionally appends the sound file, always includes name and extension, matching the expected server payload structure.
39-50: LGTM – File input with size and MIME validation.The
useSingleFileInputhook is correctly configured with:
- Allowed MIME types from shared constants
- Max size limit from shared constants
- Error callback showing formatted size using
filesizewith JEDEC standardThis aligns with the PR objective to enforce client-side validation.
apps/meteor/client/views/admin/customSounds/EditSound.tsx (4)
106-117: LGTM – File input with size and MIME validation.Correctly mirrors the AddCustomSound implementation with shared constants for allowed MIME types and max size, with appropriate error messaging using
filesizeformatter.
137-137: LGTM – File display logic correctly prioritizes selected file name.The display logic
file?.name || \${data.name}.${data.extension}` || t('None')` correctly shows the newly selected file if present, falls back to the persisted sound name with extension, and finally to the translated "None" string.
52-52: LGTM – Unsaved changes detection.The
hasUnsavedChangesmemoization correctly detects when the name differs from the original or when a new file has been selected, enabling/disabling the Save button appropriately.
72-78: Extension handling is correct – server preserves existing extension when no file is uploaded.When editing without uploading a new file, the server uses the existing extension from the database (line 247 of
apps/meteor/app/api/server/v1/custom-sounds.ts). The client correctly omits the extension field in this case.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/custom-sounds.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/custom-sounds.ts:18">
P2: This permission test targets the update suite but calls `custom-sounds.create`, so it does not verify authorization for `custom-sounds.update`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
137-137: Make the “None” fallback reachable and avoid dangling dots.At Line 137,
`${data.name}.${data.extension}`is always truthy, sot('None')never renders and empty pieces can display".".Proposed fix
- {file?.name || `${data.name}.${data.extension}` || t('None')} + {file?.name || [data.name, data.extension].filter(Boolean).join('.') || t('None')}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx` at line 137, The current JSX expression in EditSound.tsx always evaluates `${data.name}.${data.extension}` which prevents t('None') from ever showing and can produce a dangling "."; change the expression to first prefer file?.name, then if both data.name and data.extension exist render `${data.name}.${data.extension}`, otherwise render whichever of data.name or data.extension is present, and only fall back to t('None') when none are set — update the component's render (where file, data.name, data.extension, and t are used) to use conditional checks (e.g., if (file?.name) ... else if (data.name && data.extension) ... else if (data.name || data.extension) ... else t('None')) so dots only appear when both parts exist.apps/meteor/tests/end-to-end/api/custom-sounds.ts (3)
155-155: Removeasyncfromdescribecallback.Mocha's
describeblocks should not use async callbacks. Theasynckeyword here is unnecessary since the callback only contains synchronous setup (before/afterhooks anditblocks). While this may appear to work, it can cause unpredictable behavior with Mocha's test lifecycle.♻️ Suggested fix
- describe('without manage-sounds permission', async () => { + describe('without manage-sounds permission', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` at line 155, The describe block "describe('without manage-sounds permission', async () => {" is using an unnecessary async callback; remove the async keyword so it reads "describe('without manage-sounds permission', () => {" and keep the existing synchronous before/after hooks and it blocks unchanged to avoid altering Mocha's test lifecycle.
319-319: Removeasyncfromdescribecallback.Same issue as line 155 - Mocha
describeblocks should not have async callbacks.♻️ Suggested fix
- describe('without manage-sounds permission', async () => { + describe('without manage-sounds permission', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` at line 319, The describe block labeled "without manage-sounds permission" uses an async callback (describe('without manage-sounds permission', async () => { ... })); remove the async keyword from that describe callback so it becomes a synchronous function (describe('without manage-sounds permission', () => { ... })), keeping any async/await only inside individual it/test hooks or before/after hooks (e.g., move async logic into beforeEach/it blocks) to comply with Mocha usage.
82-96: Clarify intent: WAV file attached with MP3 extension.The test attaches a WAV file (
mockWavAudioPath) but setsextensionto'mp3'. If this is intentional to verify the server uses detected MIME type rather than client-provided extension, consider adding a test name or brief assertion that makes this clear. Otherwise, align the extension with the actual file type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` around lines 82 - 96, The test "should successfully create a new custom sound and return its _id" attaches mockWavAudioPath but sets field('extension','mp3'); either make the intent explicit or fix the mismatch: if you intended to verify server MIME detection, rename the test to mention "WAV file with incorrect extension" and add an assertion that the server treats it as WAV (or ignores client extension) using the response from api('custom-sounds.create'); otherwise change the field('extension') to 'wav' to match mockWavAudioPath and keep the rest (fileId3 assignment) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 35-37: The useEffect that resets name when the incoming sound
changes (useEffect -> setName(previousName || '')) must also clear the selected
replacement file to avoid leaking a previous selection; update that effect in
EditSound.tsx to call the file-state setter (e.g., setFile(null) or
setSelectedFile(undefined) — whichever the component uses) and clear any file
input ref when previousName/data changes so the file state is reset along with
setName.
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Around line 332-340: The test inside the [/custom-sounds.update] describe
block is calling the wrong endpoint; update the request invocation in that test
so it targets the update endpoint (replace api('custom-sounds.create') with
api('custom-sounds.update') in the test that currently uses
unauthorizedUserCredentials and attaches mockWavAudioPath), ensuring the rest of
the request (attach/field) remains the same so the test correctly asserts a 403
for custom-sounds.update.
- Around line 277-290: The test "should fail if the file exceeds the 5MB size
limit" calls api('custom-sounds.update') but omits the required _id, so add a
valid _id to the multipart form (e.g., add .field('_id', soundId) or create a
sound in the test setup and use its returned _id) so the request fails for
file-size validation rather than missing-id validation; update the call that
uses request.post(api('custom-sounds.update')) and the existing .field('name',
...)/.field('extension', ...) sequence to include the _id field.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Line 137: The current JSX expression in EditSound.tsx always evaluates
`${data.name}.${data.extension}` which prevents t('None') from ever showing and
can produce a dangling "."; change the expression to first prefer file?.name,
then if both data.name and data.extension exist render
`${data.name}.${data.extension}`, otherwise render whichever of data.name or
data.extension is present, and only fall back to t('None') when none are set —
update the component's render (where file, data.name, data.extension, and t are
used) to use conditional checks (e.g., if (file?.name) ... else if (data.name &&
data.extension) ... else if (data.name || data.extension) ... else t('None')) so
dots only appear when both parts exist.
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Line 155: The describe block "describe('without manage-sounds permission',
async () => {" is using an unnecessary async callback; remove the async keyword
so it reads "describe('without manage-sounds permission', () => {" and keep the
existing synchronous before/after hooks and it blocks unchanged to avoid
altering Mocha's test lifecycle.
- Line 319: The describe block labeled "without manage-sounds permission" uses
an async callback (describe('without manage-sounds permission', async () => {
... })); remove the async keyword from that describe callback so it becomes a
synchronous function (describe('without manage-sounds permission', () => { ...
})), keeping any async/await only inside individual it/test hooks or
before/after hooks (e.g., move async logic into beforeEach/it blocks) to comply
with Mocha usage.
- Around line 82-96: The test "should successfully create a new custom sound and
return its _id" attaches mockWavAudioPath but sets field('extension','mp3');
either make the intent explicit or fix the mismatch: if you intended to verify
server MIME detection, rename the test to mention "WAV file with incorrect
extension" and add an assertion that the server treats it as WAV (or ignores
client extension) using the response from api('custom-sounds.create'); otherwise
change the field('extension') to 'wav' to match mockWavAudioPath and keep the
rest (fileId3 assignment) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fc9ab8e-e821-45bb-8ce8-c25d8c417d7a
⛔ Files ignored due to path filters (1)
apps/meteor/tests/mocks/files/audio_mock.mp3is excluded by!**/*.mp3
📒 Files selected for processing (5)
apps/meteor/app/api/server/v1/custom-sounds.tsapps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.tsapps/meteor/app/custom-sounds/server/lib/uploadCustomSound.tsapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/tests/end-to-end/api/custom-sounds.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/meteor/app/custom-sounds/server/lib/uploadCustomSound.ts
- apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts
- apps/meteor/app/api/server/v1/custom-sounds.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/client/views/admin/customSounds/EditSound.tsx
🧠 Learnings (29)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-12T17:12:49.121Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:28-33
Timestamp: 2026-03-12T17:12:49.121Z
Learning: Rocket.Chat — apps/meteor/client/lib/chats/flows/uploadFiles.ts: When E2E_Enable_Encrypt_Files is disabled, plaintext file uploads are allowed in E2E rooms; this fallback is expected and should not be flagged as a security regression.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-23T17:53:18.785Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-06T18:10:23.330Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:23.330Z
Learning: In the RocketChat/Rocket.Chat `packages/gazzodown` package and more broadly, the HTML `<code>` element has an implicit ARIA role of `code` per WAI-ARIA 1.3, and `testing-library/dom` / jsdom supports it. Therefore, `screen.getByRole('code')` / `screen.findByRole('code')` correctly locates `<code>` elements without needing an explicit `role="code"` attribute. Do NOT flag `findByRole('code')` as invalid in future reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/client/views/admin/customSounds/EditSound.tsx
🔇 Additional comments (5)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
63-71: Validation short-circuit before upload is solid.Good guard here: showing the first invalid field and returning early prevents avoidable upload calls.
apps/meteor/tests/end-to-end/api/custom-sounds.ts (4)
14-31: LGTM!The
createCustomSoundhelper is well-structured, properly typed, and correctly uses the new REST endpoint pattern for creating custom sounds.
64-71: LGTM!Proper cleanup in
afterhooks ensures test isolation by deleting created sounds.
509-516: LGTM!Storage settings tests correctly use the new
createCustomSoundhelper, maintaining consistency with the REST endpoint migration.
102-115: The test expectation is correct.MeteorError('error-file-too-large')formats the message as[error-file-too-large](perMeteorError.tsline 11:super(\[${String(error)}]`)), and this message is properly passed through toAPI.v1.failure()and set asres.body.error` in the response.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (3)
320-320: Removeasyncfromdescribecallback.Same issue as line 155—Mocha ignores the returned promise from async
describecallbacks.♻️ Proposed fix
- describe('without manage-sounds permission', async () => { + describe('without manage-sounds permission', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` at line 320, The describe block callback "describe('without manage-sounds permission', async () => { ... })" is declared async which Mocha will ignore; remove the async keyword so it becomes a plain synchronous callback (describe('without manage-sounds permission', () => { ... })) and ensure any async setup inside uses before/after hooks or returns/promises from those hooks or individual it() tests rather than the describe callback itself.
155-155: Removeasyncfromdescribecallback.Mocha's
describedoes not await async callbacks—the returned promise is ignored. This can causebefore/afterhooks inside to execute unpredictably if the callback were to contain top-level awaits.♻️ Proposed fix
- describe('without manage-sounds permission', async () => { + describe('without manage-sounds permission', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` at line 155, The describe callback for the test suite "without manage-sounds permission" is declared async but Mocha does not await describe callbacks; remove the async keyword from the describe callback (the arrow function passed to describe('without manage-sounds permission', async () => { ... })) so the block uses a plain synchronous function and let any before/after hooks handle async work properly.
333-341: Consider addingextensionfield for consistency.When attaching a sound file, other update tests include the
extensionfield. Adding it here ensures the request mirrors a typical update with file upload and tests the full validation path.♻️ Suggested change
it('should return forbidden if user does not have the manage-sounds permission', async () => { await request .post(api('custom-sounds.update')) .set(unauthorizedUserCredentials) .attach('sound', mockWavAudioPath) .field('_id', fileId) .field('name', `forbidden-case`) + .field('extension', 'wav') .expect(403); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts` around lines 333 - 341, The test "should return forbidden if user does not have the manage-sounds permission" is missing the 'extension' form field that other upload/update tests include; update the request built against api('custom-sounds.update') (using unauthorizedUserCredentials, .attach('sound', mockWavAudioPath) and .field('_id', fileId)) to also include .field('extension', 'wav') so the request matches typical update payloads and exercises the same validation path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Around line 195-197: The after hook currently only restores the name for
fileId via request.post(api('custom-sounds.update')) with field('name',
previousFileName) but does not restore the file extension changed in the test;
update the after hook to also restore the original extension (e.g. include
field('extension', previousExtension) or restore the full original filename
including extension) by capturing previousExtension before the test and sending
it alongside previousFileName in the api('custom-sounds.update') call for
fileId.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Line 320: The describe block callback "describe('without manage-sounds
permission', async () => { ... })" is declared async which Mocha will ignore;
remove the async keyword so it becomes a plain synchronous callback
(describe('without manage-sounds permission', () => { ... })) and ensure any
async setup inside uses before/after hooks or returns/promises from those hooks
or individual it() tests rather than the describe callback itself.
- Line 155: The describe callback for the test suite "without manage-sounds
permission" is declared async but Mocha does not await describe callbacks;
remove the async keyword from the describe callback (the arrow function passed
to describe('without manage-sounds permission', async () => { ... })) so the
block uses a plain synchronous function and let any before/after hooks handle
async work properly.
- Around line 333-341: The test "should return forbidden if user does not have
the manage-sounds permission" is missing the 'extension' form field that other
upload/update tests include; update the request built against
api('custom-sounds.update') (using unauthorizedUserCredentials, .attach('sound',
mockWavAudioPath) and .field('_id', fileId)) to also include .field('extension',
'wav') so the request matches typical update payloads and exercises the same
validation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 284d788f-285d-4541-b8f6-12081314c7cf
📒 Files selected for processing (2)
apps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/tests/end-to-end/api/custom-sounds.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/customSounds/EditSound.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-sounds.ts
🔇 Additional comments (7)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (7)
14-31: LGTM!The
createCustomSoundhelper correctly uses the new REST API endpoint with multipart form data and properly extracts the created sound ID.
48-72: LGTM!Test setup correctly initializes fixtures using the new API and ensures cleanup via the
afterhook, maintaining test isolation.
345-395: LGTM!List endpoint tests properly cover pagination, offset, and name filtering scenarios.
397-451: LGTM!Access control tests properly verify forbidden access, not found scenarios, caching headers, and conditional request handling with
if-modified-since.
453-504: LGTM!Comprehensive test coverage including authentication, validation, and NoSQL injection protection aligns well with the security requirements from the linked issue (CORE-1863).
506-577: LGTM!Storage reactivity tests effectively verify that switching between GridFS and FileSystem backends correctly resolves files from the active storage only.
82-96: No action needed—test correctly validates server MIME type checking.The API endpoint validates the actual file's MIME type (via
getUploadFormData) againstCUSTOM_SOUND_ALLOWED_MIME_TYPES, regardless of the client-declared extension. The test uploads a real.wavfile withextension: 'mp3'which is intentional: server-side validation checks file content, not declared extension. The mismatch does not indicate a validation gap.
nazabucciarelli
force-pushed
the
feat/custom-sound-create-endpoint
branch
from
a0778ec to
e9b1812
Compare
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/client/views/admin/customSounds/EditSound.tsx">
<violation number="1" location="apps/meteor/client/views/admin/customSounds/EditSound.tsx:134">
P2: Using `t('Sound File')` here causes an i18n regression because that key is missing in most locale files.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ricardogarim
requested changes
Mar 24, 2026
Member
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
even though some of the comments come from code you’re copying over, since you’re refactoring this feature and introducing new endpoints, it makes sense to update them as well - wdyt?
apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/custom-sounds/server/lib/insertOrUpdateSound.ts
Outdated
Show resolved
Hide resolved
ricardogarim
requested changes
Mar 24, 2026
nazabucciarelli
changed the title
ricardogarim
approved these changes
Mar 31, 2026
nazabucciarelli
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
custom-sounds.createandcustom-sounds.updateAPI endpoints, reusing the same logic we had before on theinsertOrUpdateSoundanduploadCustomSoundMeteor methods plus some improvements.['audio/mpeg', 'audio/mp3', 'audio/wav', 'audio/x-wav'], as concluded with product team.insertOrUpdateSoundanduploadCustomSoundMeteor methods were updated with the new endpoints usage.Notes:
audio_mock.mp3audio file was generated programmatically for testing purposes. It contains synthetic silence and is free of copyright and DRM restrictions.insertOrUpdateSoundanduploadCustomSound: chore: deprecate insertOrUpdateSound and uploadCustomSound meteor methods #39725Issue(s)
CORE-1863 Custom Sounds endpoint allows arbitrary file upload (SVG accepted) with no size limit
CORE-1899 Implement API tests for Custom Sounds update, creation
Steps to test or reproduce
To test this we could just log in as admin users (with the
manage-soundpermission) and play with creation and update of custom sounds. It'd be needed to test the API directly, without the client and check how different MIME types and file sizes above 5 mb are rejected.Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests