fix: remove unsafe any in AddCustomSound and EditSound#38940
fix: remove unsafe any in AddCustomSound and EditSound#38940Agarwalchetan wants to merge 6 commits intoRocketChat:developfrom
any in AddCustomSound and EditSound#38940Conversation
Relax createSoundData soundFile param to { name: string } since only .name is used.
AddCustomSound: soundFile: any -> File, update state type, add undefined guard.
EditSound: add SoundFile union, narrow with instanceof File before validate/FileReader.
Fixes FIXME comments in both components.
|
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 |
|
WalkthroughSwitches custom-sounds admin UI to use File-typed sound state, differentiates uploaded Files from existing sounds in save/validation flows, narrows createSoundData parameter type, adds Save-button and runtime guards to prevent saving without name and file, and adds test mocks for i18n/toast. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Agarwalchetan
changed the title
any type in AddCustomSound and EditSound (customSounds admin)any in AddCustomSound and EditSound
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all 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/AddCustomSound.tsx">
<violation number="1" location="apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx:75">
P2: Early return on missing sound file bypasses validation/toast errors, making Save a silent no-op when no file is selected (regression of required-field feedback).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
21-27:ExistingSoundduplicatesEditSoundProps['data']— consider unifying.
ExistingSoundis structurally identical to the inlinedatatype inEditSoundProps. Additionally, the existinguseStatetype annotation (lines 38–45) now manually duplicates theSoundFileunion instead of referencing it.♻️ Suggested consolidation
-type ExistingSound = { - _id: string; - name: string; - extension?: string; -}; - -type SoundFile = File | ExistingSound; type EditSoundProps = { close?: () => void; onChange: () => void; - data: { - _id: string; - name: string; - extension?: string; - }; + data: ExistingSound; }; + +type ExistingSound = { + _id: string; + name: string; + extension?: string; +}; + +type SoundFile = File | ExistingSound;And update the
useStateto use the alias:- const [sound, setSound] = useState< - | { - _id: string; - name: string; - extension?: string; - } - | File - >(() => data); + const [sound, setSound] = useState<SoundFile>(() => data);🤖 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 21 - 27, ExistingSound duplicates the inline type used as EditSoundProps['data'] and SoundFile union was re-declared instead of being reused; replace the inline data type in EditSoundProps with a reference to the ExistingSound alias (or remove ExistingSound and export/reuse EditSoundProps['data'] as a single shared type) and update the useState type annotation to reference the SoundFile alias rather than re-declaring the union, ensuring components like EditSound and any state variables consistently use ExistingSound, SoundFile and EditSoundProps['data'] as the single source of truth.
🤖 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/AddCustomSound.tsx`:
- Around line 75-77: The current early return in AddCustomSound's saveAction
causes a silent no-op when sound is undefined; instead disable the Save action
at the source: remove the silent-return behavior in saveAction/handleSave and
change the Save button's disabled prop (the component rendering Save in
AddCustomSound) to be true when sound is falsy or validation errors exist (e.g.,
disabled={!sound || hasValidationErrors}). If you prefer the alternate fix,
replace the return inside saveAction with dispatching the existing error toast
(used by handleSave's catch) so users get feedback when no file is selected;
reference saveAction, validate, handleSave and the Save button component to
implement the change.
---
Nitpick comments:
In `@apps/meteor/client/views/admin/customSounds/EditSound.tsx`:
- Around line 21-27: ExistingSound duplicates the inline type used as
EditSoundProps['data'] and SoundFile union was re-declared instead of being
reused; replace the inline data type in EditSoundProps with a reference to the
ExistingSound alias (or remove ExistingSound and export/reuse
EditSoundProps['data'] as a single shared type) and update the useState type
annotation to reference the SoundFile alias rather than re-declaring the union,
ensuring components like EditSound and any state variables consistently use
ExistingSound, SoundFile and EditSoundProps['data'] as the single source of
truth.
ℹ️ 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
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/client/views/admin/customSounds/lib.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsx
🧠 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 (2)
createSoundData(32-61)validate(10-30)
🔇 Additional comments (4)
apps/meteor/client/views/admin/customSounds/lib.ts (1)
33-33: LGTM — correct parameter narrowing.Relaxing
soundFilefromICustomSoundFileto{ name: string }is accurate: onlysoundFile.nameis accessed inside the function body.ICustomSoundFilestill servesvalidate, so no dead code is introduced.apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
63-70: LGTM —instanceof Filenarrowing is correct.
soundFileis correctly resolved tosound(aFile) orundefinedbefore being forwarded tovalidate, and the upload branch is gated oninstanceof File. Theextensionfield threaded throughpreviousData(line 68) is accepted bycreateSoundData's type but is never read by the function (it always derives extension fromsoundFile.name), so the computed value is silently discarded. This is a pre-existing quirk oflib.ts'spreviousDatashape and not introduced by this PR, but worth noting iflib.tsis refactored later.apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (2)
22-22: LGTM —useState<File>()is clean.
34-34: LGTM —soundFile: Filecorrectly replacesany.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/gazzodown/src/Markup.spec.tsx (1)
11-17: Usejest.requireActualto avoid silently clobbering other module exports.Both mocks replace the entire module with a single-property object. Any other named export from
react-i18next(e.g.Trans) or@rocket.chat/ui-contexts(e.g.useEndpoint, constants, types re-exported as values) will resolve toundefinedinside the test environment. The idiomatic fix is to spread the actual module and override only the targeted export:{ ...jest.requireActual('module'), targetHook: mockImpl }.♻️ Proposed fix
-jest.mock('react-i18next', () => ({ - useTranslation: () => ({ t: (key: string) => key }), -})); - -jest.mock('@rocket.chat/ui-contexts', () => ({ - useToastMessageDispatch: () => (): void => undefined, -})); +jest.mock('react-i18next', () => ({ + ...jest.requireActual('react-i18next'), + useTranslation: () => ({ t: (key: string) => key }), +})); + +jest.mock('@rocket.chat/ui-contexts', () => ({ + ...jest.requireActual('@rocket.chat/ui-contexts'), + useToastMessageDispatch: () => (): void => undefined, +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/gazzodown/src/Markup.spec.tsx` around lines 11 - 17, The current tests fully replace the react-i18next and `@rocket.chat/ui-contexts` modules which can clobber other exports; update the mocks to spread the real module and override only the specific hooks by using jest.requireActual('react-i18next') and jest.requireActual('@rocket.chat/ui-contexts') and return { ...actual, useTranslation: () => ({ t: (key: string) => key }) } and { ...actual, useToastMessageDispatch: () => (): void => undefined } respectively so other named exports (e.g. Trans, useEndpoint) remain available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/gazzodown/src/Markup.spec.tsx`:
- Around line 11-17: The current tests fully replace the react-i18next and
`@rocket.chat/ui-contexts` modules which can clobber other exports; update the
mocks to spread the real module and override only the specific hooks by using
jest.requireActual('react-i18next') and
jest.requireActual('@rocket.chat/ui-contexts') and return { ...actual,
useTranslation: () => ({ t: (key: string) => key }) } and { ...actual,
useToastMessageDispatch: () => (): void => undefined } respectively so other
named exports (e.g. Trans, useEndpoint) remain available.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/gazzodown/src/Markup.spec.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). (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:
packages/gazzodown/src/Markup.spec.tsx
🧠 Learnings (7)
📚 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:
packages/gazzodown/src/Markup.spec.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/gazzodown/src/Markup.spec.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/gazzodown/src/Markup.spec.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/gazzodown/src/Markup.spec.tsx
🔇 Additional comments (1)
packages/gazzodown/src/Markup.spec.tsx (1)
15-17: The mock is correctly placed and necessary.
@rocket.chat/ui-contextsis an explicit dependency of the gazzodown package, andCodeBlock.tsx(a child component rendered byMarkup) imports and usesuseToastMessageDispatchon line 32. The mock inMarkup.spec.tsxis required to test theMarkupcomponent and its child components.Likely an incorrect or invalid review comment.
KevLehman
added
the
valid
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38940 +/- ##
===========================================
+ Coverage 70.56% 70.63% +0.07%
===========================================
Files 3189 3189
Lines 112701 112717 +16
Branches 20428 20383 -45
===========================================
+ Hits 79526 79618 +92
+ Misses 31114 31053 -61
+ Partials 2061 2046 -15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Hi, just a gentle follow-up on this PR |
5 participants
Related Issue / Discussion:
Fixes FIXME comments in:
Changes:
Modified
apps/meteor/client/views/admin/customSounds/lib.tscreateSoundDatafirst parameter fromICustomSoundFileto{ name: string }.nameModified
AddCustomSound.tsxsoundFile: anywithsoundFile: FileuseState<File>()Modified
EditSound.tsxsound: anywithSoundFileunion (File | ExistingSound)instanceof Filenarrowing before:validateFileReader.typeaccessFurther Details:
Summary
This PR removes unsafe
anyusage in the custom sounds admin components and replaces it with properly narrowed TypeScript types.Key improvements:
File | ExistingSoundunion inEditSoundinstanceof Fileguards before all File-specific APIsICustomSoundFileprivate tolib.tscreateSoundDataparameter to match actual usage ({ name: string })Behaviour Before
anyallowed unsafe.typeaccessBehaviour After
anyremovedVerification
tsc --noEmitpasses for modified filesSummary by CodeRabbit
New Features
Bug Fixes
Tests
COMM-144