chore(custom-sounds): sanitize any usage in AddCustomSound and EditSound#39044
chore(custom-sounds): sanitize any usage in AddCustomSound and EditSound#39044srijnabhargav wants to merge 3 commits intoRocketChat:developfrom
Conversation
…n AddCustomSound and EditSound
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
srijnabhargav
changed the title
srijnabhargav
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
WalkthroughThis PR tightens types in the custom-sounds admin flows: AddCustomSound now treats the sound state as a File and short-circuits when none is selected; EditSound accepts File or existing sound metadata and only uploads when given a File; ICustomSoundFile.type is now optional and validations are guarded accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
41-41:⚠️ Potential issue | 🟡 Minor
''is not assignable to the declaredsoundstate type.
setSound(previousSound || '')compiles with an error because''is not assignable to{ _id: string; name: string; extension?: string } | File. SincepreviousSoundisdata || {}(always an object), the fallback is unreachable in practice, but the type error remains and contradicts the PR's strict-typing goal.🐛 Proposed fix
- setSound(previousSound || ''); + setSound(previousSound);🤖 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 41, The call setSound(previousSound || '') fails TypeScript because '' isn't compatible with the sound state type; replace the string fallback with an object matching the state type (or a type assertion) so the fallback is correctly typed. Locate the sound state and the setSound call in EditSound.tsx (the variable names are sound, setSound and previousSound/data) and change the fallback to either previousSound ?? ({} as { _id: string; name: string; extension?: string }) or provide a properly typed empty object constant, ensuring the value passed to setSound matches the declared state type.
97-100:⚠️ Potential issue | 🟠 Major
saveActionis not awaited —onChangefires before the save/upload completes.
saveAction(sound)returns aPromise; dropping theawaitmeansonChange()is called immediately regardless of whether the insert, validation, or upload succeeds or fails. Any state refresh driven byonChangewill see stale data.🐛 Proposed fix
- const handleSave = useCallback(async () => { - saveAction(sound); - onChange(); - }, [saveAction, sound, onChange]); + const handleSave = useCallback(async () => { + await saveAction(sound); + onChange(); + }, [saveAction, sound, onChange]);🤖 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` around lines 97 - 100, handleSave currently calls saveAction(sound) without awaiting, so onChange() runs before the save/upload finishes; change handleSave to await the Promise returned by saveAction(sound) and call onChange only after the await completes — i.e., wrap await saveAction(sound) in a try/catch to surface or log errors (do not swallow), then call onChange() after the await (or rethrow the error if appropriate); update the useCallback accordingly (keep dependencies saveAction, sound, onChange).
🤖 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`:
- Line 23: The current guard uses "soundFile.type && ..." which skips MIME
validation when soundFile.type is the empty string; change the guard to detect
absence of the property instead so empty-string types still run the regex check.
Replace the truthiness check with a property-existence check (e.g., use "'type'
in soundFile" or Object.prototype.hasOwnProperty.call(soundFile, 'type')) so the
FileType validation block (the if containing the
/audio\/mp3|audio\/mpeg|audio\/x-mpeg/ regex) runs for empty-string types but
still skips for non-File shapes that genuinely lack a type property.
---
Outside diff comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Line 41: The call setSound(previousSound || '') fails TypeScript because ''
isn't compatible with the sound state type; replace the string fallback with an
object matching the state type (or a type assertion) so the fallback is
correctly typed. Locate the sound state and the setSound call in EditSound.tsx
(the variable names are sound, setSound and previousSound/data) and change the
fallback to either previousSound ?? ({} as { _id: string; name: string;
extension?: string }) or provide a properly typed empty object constant,
ensuring the value passed to setSound matches the declared state type.
- Around line 97-100: handleSave currently calls saveAction(sound) without
awaiting, so onChange() runs before the save/upload finishes; change handleSave
to await the Promise returned by saveAction(sound) and call onChange only after
the await completes — i.e., wrap await saveAction(sound) in a try/catch to
surface or log errors (do not swallow), then call onChange() after the await (or
rethrow the error if appropriate); update the useCallback accordingly (keep
dependencies saveAction, sound, onChange).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.ts
📜 Review details
🧰 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/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/lib.ts
🧠 Learnings (2)
📚 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/client/views/admin/customSounds/lib.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/views/admin/customSounds/lib.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (1)
createSoundData(32-61)
🔇 Additional comments (1)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
22-22: LGTM — type narrowing and guard are correct.State narrowed to
File | undefined,saveActionsignature tightened toFile, and the early-return guard cleanly prevents callingsaveActionwithout a file. Theawait saveAction(name, sound)on Line 78 correctly ensures errors and navigation are sequenced.Also applies to: 34-34, 75-78
ggazzo
added
the
stat: QA assured
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39044 +/- ##
===========================================
- Coverage 70.66% 70.63% -0.04%
===========================================
Files 3136 3177 +41
Lines 108599 112516 +3917
Branches 19538 20495 +957
===========================================
+ Hits 76742 79473 +2731
- Misses 29856 31015 +1159
- Partials 2001 2028 +27
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
added
the
valid
5 participants
Fixes #39043
What changed
anyin custom-sound save callbacks with strict types// FIXMEmarkerslib.tsto support both existing data and uploaded filesWhy
This improves type safety in admin custom-sound create/edit flows without changing behavior.
Impact
Summary by CodeRabbit
Bug Fixes
Refactor
COMM-144