fix: test button not playing default sound in notification preferences#40335
Open
nazabucciarelli wants to merge 4 commits intodevelopfrom
Open
fix: test button not playing default sound in notification preferences#40335nazabucciarelli wants to merge 4 commits intodevelopfrom
nazabucciarelli wants to merge 4 commits intodevelopfrom
Conversation
Contributor
|
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 |
🦋 Changeset detectedLatest commit: dd9f430 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
WalkthroughResolves desktop notification sound Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as NotificationPreferences UI
participant Pref as useUserPreference
participant Sub as useRoomSubscription
participant Sound as useCustomSound
User->>UI: Click play button
UI->>Sub: read audioNotificationValue (desktopSound)
alt desktopSound is "default"
UI->>Pref: read newMessageNotification (fallback "chime")
Pref-->>UI: userSound
UI->>Sound: play(userSound)
else desktopSound is non-"default"
Sub-->>UI: desktopSound
UI->>Sound: play(desktopSound)
end
Sound-->>UI: playback started
UI-->>User: feedback (playing)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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. Review rate limit: 6/8 reviews remaining, refill in 8 minutes and 32 seconds.Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40335 +/- ##
===========================================
- Coverage 69.94% 69.77% -0.17%
===========================================
Files 3297 3300 +3
Lines 119267 119487 +220
Branches 21515 21581 +66
===========================================
- Hits 83416 83372 -44
- Misses 32548 32807 +259
- Partials 3303 3308 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
nazabucciarelli
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes the Notifications Preferences “test” action so that choosing "default" plays the user’s configured newMessageNotification sound (instead of failing to resolve a sound source).
Changes:
- Resolve
"default"to the user’snewMessageNotificationpreference before looking up the sound inCustomSoundProvider. - Add unit tests validating
"default"resolution behavior and direct sound playback. - Add a changeset documenting the bugfix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.tsx | Map "default" to the preferred notification sound when playing. |
| apps/meteor/client/providers/CustomSoundProvider/CustomSoundProvider.spec.tsx | Adds tests for "default" resolution and normal sound playback. |
| .changeset/major-coats-smash.md | Adds release note entry for the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cardoso
previously approved these changes
Apr 30, 2026
aleksandernsilva
requested changes
Apr 30, 2026
|
|
||
| const play = useEffectEvent((soundId: ICustomSound['_id'], { volume = 1, loop = false } = {}) => { | ||
| stop(soundId); | ||
| const resolvedSoundId = soundId === 'default' ? newMessageNotification : soundId; |
Contributor
There was a problem hiding this comment.
Does it make sense to play newMessageNotification for any soundId equal to "default"?
It makes more sense for either, not accept "default" at all as soundId, and whoever is calling needs to make sure to replace it with the correct id, or at least having the default sound id fallback as an optional parameter within the options object.
Contributor
Author
There was a problem hiding this comment.
Nice catch, I've changed the approach to do the fix downstream instead of upper level, and created the proper unit test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx (2)
18-22: 💤 Low valueRemove the inline comments from the test setup.
They add noise and violate the repo rule for TypeScript/JavaScript files to avoid code comments in implementation. As per coding guidelines,
**/*.{ts,tsx,js}: Avoid code comments in the implementation.Proposed cleanup
jest.mock('../../contexts/RoomContext', () => ({ useRoom: () => ({ _id: 'GENERAL' }), - // 2. Conectamos la exportación al mock dinámico useRoomSubscription: () => mockUseRoomSubscription(), })); @@ mockUseRoomSubscription.mockReturnValue({ disableNotifications: false, muteGroupMentions: false, hideUnreadStatus: false, hideMentionStatus: false, - audioNotificationValue: undefined, // desktopSound defaults to 'default' + audioNotificationValue: undefined, }); });Also applies to: 32-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx` around lines 18 - 22, Remove the inline comments in the test mock setup: in the jest.mock block replace the commented lines with just the exports—return useRoom: () => ({ _id: 'GENERAL' }) and useRoomSubscription: () => mockUseRoomSubscription()—and similarly remove any other inline comments around useRoom, useRoomSubscription, and mockUseRoomSubscription in this spec (also apply the same cleanup for the occurrences around lines 32-40).
27-30: ⚡ Quick winAdd one case that leaves
newMessageNotificationunset.
appRoot()always defaults the preference to"chime", so this suite never exercises the real fallback path when the user preference is missing. A regression there would still pass these tests.Suggested test update
-const appRoot = (userPreferences?: Record<string, unknown>) => - mockAppRoot() - .withUserPreference('newMessageNotification', userPreferences?.newMessageNotification ?? 'chime') - .build(); +const appRoot = (userPreferences?: Record<string, unknown>) => { + const root = mockAppRoot(); + if (userPreferences?.newMessageNotification !== undefined) { + root.withUserPreference('newMessageNotification', userPreferences.newMessageNotification); + } + return root.build(); +}; @@ it('plays the user newMessageNotification preference when desktopSound is "default"', async () => { render(<NotificationPreferencesWithData />, { wrapper: appRoot({ newMessageNotification: 'chime' }), }); @@ expect(mockPlay).toHaveBeenCalledWith('chime'); }); + + it('falls back to chime when newMessageNotification is unset', async () => { + render(<NotificationPreferencesWithData />, { + wrapper: appRoot(), + }); + + await userEvent.click(screen.getByTestId('play-sound-button')); + + expect(mockPlay).toHaveBeenCalledWith('chime'); + });Also applies to: 43-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx` around lines 27 - 30, The test helper appRoot is always forcing newMessageNotification to 'chime' which prevents testing the unset/fallback path; change appRoot in NotificationPreferencesWithData.spec.tsx so it only calls .withUserPreference('newMessageNotification', ...) when userPreferences has an own value for newMessageNotification (i.e., check for property existence instead of using ?? 'chime'), then add a test case that calls appRoot() or appRoot({}) to render the component with newMessageNotification truly unset and assert the fallback behavior; update both places referenced (the helper at top and the additional tests around lines 43-53) to cover the missing-preference path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx`:
- Around line 18-22: Remove the inline comments in the test mock setup: in the
jest.mock block replace the commented lines with just the exports—return
useRoom: () => ({ _id: 'GENERAL' }) and useRoomSubscription: () =>
mockUseRoomSubscription()—and similarly remove any other inline comments around
useRoom, useRoomSubscription, and mockUseRoomSubscription in this spec (also
apply the same cleanup for the occurrences around lines 32-40).
- Around line 27-30: The test helper appRoot is always forcing
newMessageNotification to 'chime' which prevents testing the unset/fallback
path; change appRoot in NotificationPreferencesWithData.spec.tsx so it only
calls .withUserPreference('newMessageNotification', ...) when userPreferences
has an own value for newMessageNotification (i.e., check for property existence
instead of using ?? 'chime'), then add a test case that calls appRoot() or
appRoot({}) to render the component with newMessageNotification truly unset and
assert the fallback behavior; update both places referenced (the helper at top
and the additional tests around lines 43-53) to cover the missing-preference
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d214818e-c322-4b56-8e3e-90fb876d3425
📒 Files selected for processing (2)
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesForm.tsxapps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesForm.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/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
🧠 Learnings (13)
📚 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/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
📚 Learning: 2026-04-17T18:33:27.211Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39858
File: apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts:123-151
Timestamp: 2026-04-17T18:33:27.211Z
Learning: In RocketChat/Rocket.Chat (`apps/meteor/tests/e2e/apps/uikit-interactions.spec.ts`), `executeBlockActionHandler` invocations originating from a **modal** surface intentionally do NOT include a `block_action_room` (room property) in the interaction payload. Modals are not scoped to a room, so no room id is available in that context. Do not flag the absence of a room assertion in the modal block-action test as a missing coverage bug; instead, document it explicitly with a `test.step` asserting the room entry is `undefined`.
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
📚 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/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.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 `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
📚 Learning: 2026-02-24T19:22:52.984Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:52.984Z
Learning: In RocketChat/Rocket.Chat Playwright e2e tests, prefer using translated text and ARIA roles (getByRole, getByText, etc.) over data-qa locators. If translation values change, update the corresponding test locators accordingly. Never prefer data-qa locators.
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx
🔇 Additional comments (1)
apps/meteor/client/views/room/contextualBar/NotificationPreferences/NotificationPreferencesWithData.spec.tsx (1)
43-82: Looks good overall.The three playback paths are covered, the mocks are isolated per test, and the assertions match the intended sound-resolution behavior.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
nazabucciarelli
force-pushed
the
fix/sound-reproduction-for-default
branch
from
c807544 to
dd9f430
Compare
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
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)
newMessageNotificationuser preference when trying to play the default sound.Issue(s)
SUP-1027 "test sound" button don't work for Default
Steps to test or reproduce
1- Be logged in as any user
2- Go to any room
3- Click on kebab menu (options)
4- Click notification preferences
5- Under the
Desktopsection, select theDefaultoption in theSounddropdown and click on the play buttonFurther comments
Summary by CodeRabbit
Bug Fixes
Tests