fix: test button not playing default sound in notification preferences#40335
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: e7bc5a1 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
|
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:
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)
📜 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)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2026-03-06T18:10:15.268ZApplied to files:
📚 Learning: 2026-03-27T14:52:56.865ZApplied to files:
🔇 Additional comments (2)
WalkthroughFixes notification sound playback: when desktop sound is ChangesNotification Preferences — playback resolution, UI, tests, changeset
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" or undefined
UI->>Pref: read newMessageNotification (fallback "chime")
Pref-->>UI: userSound
UI->>Sound: play(userSound)
else desktopSound is explicit
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40335 +/- ##
===========================================
+ Coverage 69.94% 70.07% +0.13%
===========================================
Files 3297 3301 +4
Lines 119267 120778 +1511
Branches 21515 21619 +104
===========================================
+ Hits 83416 84638 +1222
- Misses 32548 32846 +298
+ Partials 3303 3294 -9
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
![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
aleksandernsilva
requested changes
May 4, 2026
aleksandernsilva
approved these changes
May 4, 2026
nazabucciarelli
added
the
stat: QA assured
nazabucciarelli
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
nazabucciarelli
added
stat: ready to merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
nazabucciarelli
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
nazabucciarelli
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
nazabucciarelli
removed
the
stat: QA assured
dionisio-bot
Bot
removed
the
stat: ready to merge
nazabucciarelli
added
stat: ready to merge
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
dionisio-bot
Bot
added
stat: ready to merge
nazabucciarelli
added
stat: ready to merge
ggazzo
pushed a commit
that referenced
this pull request
May 5, 2026
ricardogarim
pushed a commit
that referenced
this pull request
May 6, 2026
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