chore(admin): fix unsafe type assertions in Custom Sounds#38740
chore(admin): fix unsafe type assertions in Custom Sounds#38740NAME-ASHWANIYADAV wants to merge 8 commits intoRocketChat:developfrom
Conversation
|
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 |
|
|
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:
WalkthroughSound handling now treats selected sounds as File objects when applicable; save/edit flows detect new File instances to branch into file-read/upload vs existing-data paths. Validation accepts File or existing data and tolerates missing MIME types. A guard dispatches an error if no file is selected. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant UI as Add/Edit Component
participant Validator
participant FileReader
participant Uploader
participant Toasts
end
User->>UI: Select or edit sound (File or existing data)
UI->>Validator: validate(soundData, soundFile)
alt soundFile is File (new upload)
UI->>FileReader: read file
FileReader-->>UI: file data
UI->>Uploader: uploadCustomSound(file data)
Uploader-->>UI: upload result
UI->>Toasts: show File_uploaded toast
UI-->>User: close and return early
else existing sound (no new file)
UI->>UI: run saveAction with existing data
UI->>Toasts: show success save toast
UI-->>User: finish
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
98-101:⚠️ Potential issue | 🟠 MajorMissing
awaitonsaveAction—onChange()fires before save completes.
saveActionis async, but line 99 doesn'tawaitit. This meansonChange()(which likely refreshes the sound list) runs immediately, potentially before the insert/upload finishes. Compare withAddCustomSound.tsxline 75 which correctly usesconst result = await saveAction(...).Proposed fix
const handleSave = useCallback(async () => { - saveAction(sound); + 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 98 - 101, The handleSave callback currently calls saveAction(sound) without awaiting it, causing onChange() to run before the async save completes; update handleSave (the useCallback that calls saveAction and onChange) to await the result of saveAction (e.g., const result = await saveAction(sound)) before calling onChange(), and optionally handle errors (try/catch) or use the result if needed to ensure onChange runs only after the save finishes.apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
73-84:⚠️ Potential issue | 🟡 Minor
soundmay beundefinedbutsaveActionexpectsFile.
soundis typed asFile | undefined(line 22), butsaveActionon line 34 declaressoundFile: File. The call on line 75 (saveAction(name, sound)) would be a TypeScript error under strict null checks. Consider either guarding before the call or making the parameter optional.Option: guard before calling saveAction
const handleSave = useCallback(async () => { try { + if (!sound) { + dispatchToastMessage({ type: 'error', message: t('Required_field', { field: t('Sound File') }) }); + return; + } const result = await saveAction(name, sound);🤖 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 73 - 84, The handleSave callback calls saveAction(name, sound) while sound is typed File | undefined but saveAction expects a File; update to either (A) guard before calling in handleSave: if (!sound) { dispatchToastMessage(...); return; } then call saveAction(name, sound) so you only pass a File, or (B) change saveAction's signature (the function declared as saveAction) to accept soundFile?: File or soundFile: File | undefined and handle the undefined case inside saveAction; pick one approach and update references to saveAction and the handleSave logic accordingly.
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/customSounds/lib.ts (1)
21-27:soundFile.typeused without guarding forundefined.Since
typeis now optional inICustomSoundFile,soundFile.typecould beundefined. WhileRegExp.test(undefined)coerces to"undefined"and correctly fails the check (pushing'FileType'), this is implicit and fragile. Consider adding an explicit guard for clarity.That said, current callers only pass
Fileinstances (which always have.type) orundefined, so this is safe in practice.Optional: add explicit guard
if (soundFile) { if (!soundData.previousSound || soundData.previousSound !== soundFile) { - if (!/audio\/mp3/.test(soundFile.type) && !/audio\/mpeg/.test(soundFile.type) && !/audio\/x-mpeg/.test(soundFile.type)) { + if (!soundFile.type || (!/audio\/mp3/.test(soundFile.type) && !/audio\/mpeg/.test(soundFile.type) && !/audio\/x-mpeg/.test(soundFile.type))) { errors.push('FileType'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/admin/customSounds/lib.ts` around lines 21 - 27, The validation uses soundFile.type without an explicit undefined check; update the sound file validation in the block that references soundFile, soundData, and ICustomSoundFile so it first verifies soundFile.type exists (e.g., if (!soundFile.type) { errors.push('FileType') }) before running the regex tests, and only run the /audio\/.../.test(...) checks when soundFile.type is a non-empty string to make the intent explicit and robust.apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
54-93: Validation errors are dispatched even on success path.Lines 88–93 run unconditionally after the
if (validation.length === 0)block. When validation passes, theforEachis a no-op (empty array), so there's no user-visible bug—but it reads as though error handling was intended to be in anelsebranch. This is a pre-existing issue, not introduced by this PR.Cleaner structure with early return
async (sound: EditSoundProps['data'] | File) => { const isNewFile = sound instanceof File; const extension = isNewFile ? undefined : sound.extension; const soundData = createSoundData(sound, name, { previousName, previousSound, _id, extension: extension ?? '' }); const validation = validate(soundData, isNewFile ? sound : undefined); + + if (validation.length > 0) { + validation.forEach((invalidFieldName) => + dispatchToastMessage({ + type: 'error', + message: t('Required_field', { field: t(invalidFieldName) }), + }), + ); + return; + } + - if (validation.length === 0) { let soundId: string; try { soundId = await insertOrUpdateSound(soundData); ... - } - - validation.forEach((invalidFieldName) => - dispatchToastMessage({ - type: 'error', - message: t('Required_field', { field: t(invalidFieldName) }), - }), - ); },🤖 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 54 - 93, The validation error dispatch runs after the successful-save path in saveAction which makes the control flow confusing; change saveAction so that when validation.length === 0 you either return early after finishing the success/upload logic (including after reader.onloadend branch completes/queues work) or place the validation.forEach(...) inside an else block; locate the saveAction callback and the validation.forEach call and implement the early return or else branch so error messages are only dispatched when validation failed.
📜 Review details
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
🧰 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/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 36717
File: packages/ui-voip/src/providers/useCallSounds.ts:6-21
Timestamp: 2025-09-15T21:34:39.812Z
Learning: The voipSounds methods (playDialer, playRinger, playCallEnded) from useCustomSound return proper offCallbackHandler cleanup functions, not void as some type definitions might suggest.
📚 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 (2)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(32-61)validate(10-30)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(32-61)validate(10-30)
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/views/admin/customSounds/lib.ts (1)
3-7: LGTM on makingtypeoptional.This aligns with the updated caller pattern where
Fileobjects (which always carry.type) are passed for validation, while existing sound data objects (without.type) are not passed tovalidateat all.apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
27-27: Good improvements: properpreviousSoundinitialization and union type forsoundstate.Using
datadirectly instead of an empty object fallback, and theEditSoundProps['data'] | Fileunion type, are clean improvements that enable proper type narrowing downstream.Also applies to: 30-37
146-146: Good i18n fix —t('None')instead of hardcoded'none'.apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
22-22: Good type improvements —Filestate and removal of explicit cast onvalidate.Using
Filedirectly aligns with whatuseSingleFileInputprovides, and removing the cast onvalidate's return lets TypeScript enforce correctness naturally.Also applies to: 33-36
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx`:
- Around line 73-84: The handleSave callback calls saveAction(name, sound) while
sound is typed File | undefined but saveAction expects a File; update to either
(A) guard before calling in handleSave: if (!sound) { dispatchToastMessage(...);
return; } then call saveAction(name, sound) so you only pass a File, or (B)
change saveAction's signature (the function declared as saveAction) to accept
soundFile?: File or soundFile: File | undefined and handle the undefined case
inside saveAction; pick one approach and update references to saveAction and the
handleSave logic accordingly.
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 98-101: The handleSave callback currently calls saveAction(sound)
without awaiting it, causing onChange() to run before the async save completes;
update handleSave (the useCallback that calls saveAction and onChange) to await
the result of saveAction (e.g., const result = await saveAction(sound)) before
calling onChange(), and optionally handle errors (try/catch) or use the result
if needed to ensure onChange runs only after the save finishes.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 54-93: The validation error dispatch runs after the
successful-save path in saveAction which makes the control flow confusing;
change saveAction so that when validation.length === 0 you either return early
after finishing the success/upload logic (including after reader.onloadend
branch completes/queues work) or place the validation.forEach(...) inside an
else block; locate the saveAction callback and the validation.forEach call and
implement the early return or else branch so error messages are only dispatched
when validation failed.
In `@apps/meteor/client/views/admin/customSounds/lib.ts`:
- Around line 21-27: The validation uses soundFile.type without an explicit
undefined check; update the sound file validation in the block that references
soundFile, soundData, and ICustomSoundFile so it first verifies soundFile.type
exists (e.g., if (!soundFile.type) { errors.push('FileType') }) before running
the regex tests, and only run the /audio\/.../.test(...) checks when
soundFile.type is a non-empty string to make the intent explicit and robust.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
100-103:⚠️ Potential issue | 🟠 Major
onChange()is called even whensaveActionencounters validation errors.
saveActiondispatches error toasts on validation failure (Lines 90-95) but neither throws nor returns a distinguishing value. Consequently,handleSavealways callsonChange()on Line 102, triggering a data refresh even when nothing was saved.Consider having
saveActionreturn a boolean or throw on failure sohandleSavecan skip theonChange()call:Proposed fix (return boolean from saveAction)
Inside
saveAction, returntrueon success andfalseon validation failure:close?.(); - return; + return true; } validation.forEach((invalidFieldName) => @@ ... ); + return false; },Then in
handleSave:const handleSave = useCallback(async () => { - await saveAction(sound); - onChange(); + const saved = await saveAction(sound); + if (saved) { + 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 100 - 103, handleSave currently always calls onChange() after awaiting saveAction(sound), causing refreshes even when saveAction fails validation; update saveAction to return a success boolean (true on success, false on validation failure) or throw on error, and then change handleSave to await the call and only call onChange() when saveAction succeeded (i.e., if (await saveAction(sound)) onChange()). Reference the saveAction function and the handleSave callback so you can update both the action to return/throw and the handleSave logic to conditionally invoke onChange().
86-97:⚠️ Potential issue | 🟡 Minor
closeis used insidesaveActionbut missing from the dependency array.
close?.()is called on Line 86 butcloseis not included in the dependency array on Line 97. Same issue as inAddCustomSound.tsx.Proposed fix
- [_id, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound], + [_id, close, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound],🤖 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 86 - 97, The saveAction callback uses close (calls close?.()) but close is not listed in its dependency array; update the dependency array for the saveAction hook to include close so the callback captures the current close reference. Locate the saveAction definition and its dependency array (the array containing _id, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound) and add close to that array; mirror the same fix applied in AddCustomSound where close was added to the dependencies.apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
65-71:⚠️ Potential issue | 🟡 Minor
closeis used insidesaveActionbut missing from the dependency array.
closeis called on Line 65 but is not listed in the dependency array on Line 71. This is a stale closure risk — ifcloseever changes identity across renders, the callback will reference the old value. Whilecloselikely doesn't change often, this is still a correctness issue for exhaustive deps.Proposed fix
- [dispatchToastMessage, insertOrUpdateSound, t, uploadCustomSound], + [close, dispatchToastMessage, insertOrUpdateSound, t, uploadCustomSound],🤖 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 65 - 71, The saveAction callback references close but it is missing from its dependency array (currently [dispatchToastMessage, insertOrUpdateSound, t, uploadCustomSound]), which creates a stale-closure risk; update the useCallback dependency list for saveAction to include close (or otherwise ensure close is stable) so the latest close is captured when saveAction is recreated—look for the saveAction function where close() is called inside the try block and add close to the dependency array.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsx
🧰 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.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(32-61)validate(10-30)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(32-61)validate(10-30)
⏰ 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 comments (4)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (2)
22-22: Good improvement:Filetype instead of untyped object.Using
useState<File>()properly reflects theuseSingleFileInputcallback signature and eliminates the previous unsafe typing.
74-77: Good guard for missing sound file.The early return with an error toast when no file is selected prevents a runtime error that would otherwise occur when passing
undefinedtosaveAction.apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
55-59: Good use ofinstanceof Filefor type narrowing.The
instanceof Filecheck cleanly discriminates between the two union branches, allowing safe property access without type assertions. The conditional validation call (Line 59) correctly skips file-type validation when no new file is uploaded.
148-148: Good i18n improvement — replacing hardcoded'none'witht('None').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx`:
- Around line 65-71: The saveAction callback references close but it is missing
from its dependency array (currently [dispatchToastMessage, insertOrUpdateSound,
t, uploadCustomSound]), which creates a stale-closure risk; update the
useCallback dependency list for saveAction to include close (or otherwise ensure
close is stable) so the latest close is captured when saveAction is
recreated—look for the saveAction function where close() is called inside the
try block and add close to the dependency array.
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 100-103: handleSave currently always calls onChange() after
awaiting saveAction(sound), causing refreshes even when saveAction fails
validation; update saveAction to return a success boolean (true on success,
false on validation failure) or throw on error, and then change handleSave to
await the call and only call onChange() when saveAction succeeded (i.e., if
(await saveAction(sound)) onChange()). Reference the saveAction function and the
handleSave callback so you can update both the action to return/throw and the
handleSave logic to conditionally invoke onChange().
- Around line 86-97: The saveAction callback uses close (calls close?.()) but
close is not listed in its dependency array; update the dependency array for the
saveAction hook to include close so the callback captures the current close
reference. Locate the saveAction definition and its dependency array (the array
containing _id, dispatchToastMessage, insertOrUpdateSound, name, previousName,
previousSound, t, uploadCustomSound) and add close to that array; mirror the
same fix applied in AddCustomSound where close was added to the dependencies.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38740 +/- ##
===========================================
- Coverage 70.51% 70.49% -0.02%
===========================================
Files 3176 3176
Lines 111139 111139
Branches 20050 20038 -12
===========================================
- Hits 78367 78348 -19
- Misses 30721 30748 +27
+ Partials 2051 2043 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
100-103:⚠️ Potential issue | 🟠 Major
onChange()fires afterclose(), risking a call on an unmounted component.
saveActioncallsclose()internally (line 86) before returning. ThenhandleSaveunconditionally callsonChange()on line 102 — after the contextual bar is already closed and the component may be unmounted. Additionally,onChange()is called even when validation fails (saveAction shows toasts but doesn't throw).Consider moving
onChange()insidesaveActionbefore theclose()call (similar to howhandleDeleteButtonClickhandles it), or havingsaveActionreturn a success indicator.Proposed fix: move onChange into saveAction
const saveAction = useCallback( async (sound: EditSoundProps['data'] | File) => { const isNewFile = sound instanceof File; const extension = isNewFile ? undefined : sound.extension; const soundData = createSoundData(sound, name, { previousName, previousSound, _id, extension: extension ?? '' }); const validation = validate(soundData, isNewFile ? sound : undefined); if (validation.length === 0) { let soundId: string; try { soundId = await insertOrUpdateSound(soundData); } catch (error) { dispatchToastMessage({ type: 'error', message: error }); return; } soundData._id = soundId; soundData.random = Math.round(Math.random() * 1000); if (isNewFile) { dispatchToastMessage({ type: 'success', message: t('Uploading_file') }); const reader = new FileReader(); reader.readAsBinaryString(sound); reader.onloadend = (): void => { try { uploadCustomSound(reader.result as string, sound.type, { ...soundData, _id: soundId }); return dispatchToastMessage({ type: 'success', message: t('File_uploaded') }); } catch (error) { dispatchToastMessage({ type: 'error', message: error }); } }; } + onChange(); close?.(); return; } validation.forEach((invalidFieldName) => dispatchToastMessage({ type: 'error', message: t('Required_field', { field: t(invalidFieldName) }), }), ); }, - [_id, close, dispatchToastMessage, insertOrUpdateSound, name, previousName, previousSound, t, uploadCustomSound], + [_id, close, dispatchToastMessage, insertOrUpdateSound, name, onChange, previousName, previousSound, t, uploadCustomSound], ); const handleSave = useCallback(async () => { await saveAction(sound); - onChange(); - }, [saveAction, sound, onChange]); + }, [saveAction, sound]);🤖 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 100 - 103, handleSave currently calls onChange() after awaiting saveAction(sound) but saveAction calls close() internally and may not indicate success (validation failures show toasts but don't throw), risking onChange executing on an unmounted component; either move the onChange() call into saveAction so it runs before close() (mirroring handleDeleteButtonClick), or modify saveAction to return a success indicator (e.g., boolean) and change handleSave to call onChange() only when that indicator is true and before calling/after ensuring close() timing; update references to handleSave, saveAction, onChange, and close accordingly so onChange runs only on successful save and before the contextual bar is closed.
🧹 Nitpick comments (2)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
27-27:useMemo(() => data, [data])is a no-op.This memo returns
dataunchanged and re-runs wheneverdatachanges — it's equivalent to just usingdatadirectly. If the intent is to preserve a stable reference,datawould need to be deeply compared or derived differently.🤖 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 27, The line using useMemo to create previousSound is a no-op because useMemo(() => data, [data]) just returns data directly; replace it with one of two fixes: (A) if you only need the current data, drop useMemo and use data directly (remove previousSound or set const previousSound = data), or (B) if you need a stable previous reference, use a ref and effect to capture prior value (create const previousSoundRef = useRef<typeof data | null>(null); update previousSoundRef.current inside a useEffect that runs on data changes and read previousSoundRef.current where needed), or (C) use a deep-compare memo helper (e.g., useDeepCompareMemo or custom deepCompare) to memoize derived values — update references to previousSound accordingly (replace previousSound creation and any consumers).apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
74-89:onChange()is called even whensaveActionfails silently.When
insertOrUpdateSoundthrows insidesaveAction, the error is caught internally andundefinedis returned. Back inhandleSave,goToNewis skipped (line 84) butonChange()on line 85 still fires, potentially triggering a list refresh despite nothing actually changing. This is pre-existing behavior, but since you're already touching this flow, consider guardingonChangebehindresult:Proposed fix
const handleSave = useCallback(async () => { if (!sound) { return dispatchToastMessage({ type: 'error', message: t('Required_field', { field: t('Sound_File_mp3') }) }); } try { const result = await saveAction(name, sound); if (result) { dispatchToastMessage({ type: 'success', message: t('Custom_Sound_Saved_Successfully') }); + goToNew(result); + onChange(); } - result && goToNew(result); - onChange(); } catch (error) { dispatchToastMessage({ type: 'error', message: error }); } }, [dispatchToastMessage, goToNew, name, onChange, saveAction, sound, t]);🤖 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 74 - 89, handleSave currently calls onChange() regardless of whether saveAction returned a successful result (insertOrUpdateSound may swallow errors and return undefined); change the flow in handleSave so onChange() is only invoked when result is truthy (i.e., after the existing result && goToNew(result) line), ensuring onChange is not called on failed or silent saveAction failures; keep the existing try/catch and toast behavior intact and reference handleSave, saveAction, insertOrUpdateSound, goToNew, and onChange to locate the change.
📜 Review details
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
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/admin/customSounds/lib.ts
🧰 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/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsx
🧬 Code graph analysis (2)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(35-64)validate(10-33)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
apps/meteor/client/views/admin/customSounds/lib.ts (2)
createSoundData(35-64)validate(10-33)
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (2)
22-22: Good improvement: properFiletyping for sound state.Clean change from an untyped object to
File, which correctly matches whatuseSingleFileInputprovides.
34-36: LGTM — typed parameter and removed unnecessary cast.The
Fileparameter type and directvalidatecall without casting are cleaner.apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
55-59: Type narrowing withinstanceof Fileis clean and correct.The
instanceof Filecheck properly narrows the union type, allowing safe access tosound.extensionin the non-File branch and correct dispatch tovalidate/createSoundData.
148-148: Good i18n fix —t('None')instead of hardcoded'none'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 100-103: handleSave currently calls onChange() after awaiting
saveAction(sound) but saveAction calls close() internally and may not indicate
success (validation failures show toasts but don't throw), risking onChange
executing on an unmounted component; either move the onChange() call into
saveAction so it runs before close() (mirroring handleDeleteButtonClick), or
modify saveAction to return a success indicator (e.g., boolean) and change
handleSave to call onChange() only when that indicator is true and before
calling/after ensuring close() timing; update references to handleSave,
saveAction, onChange, and close accordingly so onChange runs only on successful
save and before the contextual bar is closed.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx`:
- Around line 74-89: handleSave currently calls onChange() regardless of whether
saveAction returned a successful result (insertOrUpdateSound may swallow errors
and return undefined); change the flow in handleSave so onChange() is only
invoked when result is truthy (i.e., after the existing result &&
goToNew(result) line), ensuring onChange is not called on failed or silent
saveAction failures; keep the existing try/catch and toast behavior intact and
reference handleSave, saveAction, insertOrUpdateSound, goToNew, and onChange to
locate the change.
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Line 27: The line using useMemo to create previousSound is a no-op because
useMemo(() => data, [data]) just returns data directly; replace it with one of
two fixes: (A) if you only need the current data, drop useMemo and use data
directly (remove previousSound or set const previousSound = data), or (B) if you
need a stable previous reference, use a ref and effect to capture prior value
(create const previousSoundRef = useRef<typeof data | null>(null); update
previousSoundRef.current inside a useEffect that runs on data changes and read
previousSoundRef.current where needed), or (C) use a deep-compare memo helper
(e.g., useDeepCompareMemo or custom deepCompare) to memoize derived values —
update references to previousSound accordingly (replace previousSound creation
and any consumers).
KevLehman
added
the
contrib: valid
2 participants
Proposed changes
This PR addresses unsafe type assertions (
as any,FIXME) and improves type safety in the Custom Sounds administration components (AddCustomSound.tsx, EditSound.tsx, and lib.ts).Key Fixes:
AddCustomSound.tsx:
soundto use File type instead of{ name: string }, matching theuseSingleFileInputhook return type.saveActionto acceptsoundFile: Fileinstead ofany.EditSound.tsx:
saveActionto accept union typeEditSoundProps['data'] | Fileinstead ofany.instanceof Fileto safely access properties liketypeandextension.previousSoundstate was initialized with an empty object{}(invalid type) instead ofdata.'none'string with t('None') for better internationalization support.lib.ts:
typeoptional, supporting both file uploads (with type) and existing data (without type), as createSoundData function only requiresname.Issue(s)
#38739
How to test or reproduce
Screenshots
No visual changes, purely refactoring for type safety.
Types of changes
Summary by CodeRabbit
Bug Fixes
Improvements