Fix mobile notification sound#7327
Conversation
Signed-off-by: psrsingh <psr.singh336@gmail.com>
|
|
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds audio preference fields to room/subscription types and DB, a migration, a playNotificationSound helper using Expo AV, maps room preference into subscriptions, and makes InAppNotification fetch the subscription and call the helper to play the chosen sound when an in-app notification arrives. ChangesIn-app notification sound playback
Sequence DiagramsequenceDiagram
participant NotificationEvent
participant InAppNotification
participant SubscriptionRepo
participant PlayHelper
NotificationEvent->>InAppNotification: show(payload with rid)
InAppNotification->>SubscriptionRepo: getSubscriptionByRoomId(rid)
SubscriptionRepo-->>InAppNotification: subscription (includes audioNotificationValue)
alt audioNotificationValue present
InAppNotification->>PlayHelper: playNotificationSound(audioNotificationValue)
PlayHelper-->>InAppNotification: playback started
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/InAppNotification/index.tsx`:
- Around line 49-52: Wrap the async subscription lookup and the notification
sound call in a try/catch so rejections cannot prevent the banner render: call
getSubscriptionByRoomId(payload.rid) inside a try block (or assign a safe
default on failure) and catch/log any error, and await
playNotificationSound(sub.audioNotificationValue) inside its own try/catch (or
chain .catch) to prevent an unhandled promise; keep the banner rendering logic
independent of these operations so failures only affect sound, not rendering.
In `@app/lib/methods/helpers/mergeSubscriptionsRooms.ts`:
- Around line 113-118: The current logic in mergeSubscriptionsRooms.ts sets
mergedSubscription.audioNotificationValue from room.audioNotificationValue and
then unconditionally overwrites it if room.audioNotificationsValue exists,
letting the legacy key win; change this to use the legacy key only as a fallback
by making the second branch conditional (else if or check that
'audioNotificationValue' is not present/undefined) so that
mergedSubscription.audioNotificationValue is set from
room.audioNotificationValue when present, and only set from
room.audioNotificationsValue when the canonical room.audioNotificationValue is
absent.
In `@app/lib/methods/helpers/playNotificationSound.ts`:
- Around line 34-66: The playback setup can leak if createAsync succeeds but
playAsync throws because unload() is block-scoped; refactor
playNotificationSound so the created Sound instance and the unload function are
declared in the outer scope (e.g., let sound: Audio.Sound | null = null; let
unload = () => {};), assign them after await Audio.Sound.createAsync(asset), and
in the catch block call unload() if a sound was created; ensure unload clears
watchdog, guards with an unloaded flag, and calls sound.unloadAsync().catch(...)
for best-effort cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e4c3a6e-b6e2-4380-b181-23d8fa6bc8ab
📒 Files selected for processing (8)
app/containers/InAppNotification/index.tsxapp/definitions/IRoom.tsapp/definitions/ISubscription.tsapp/lib/database/model/Subscription.jsapp/lib/database/model/migrations.jsapp/lib/database/schema/app.jsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/methods/helpers/playNotificationSound.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/database/model/Subscription.jsapp/definitions/ISubscription.tsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/database/schema/app.jsapp/lib/methods/helpers/playNotificationSound.tsapp/lib/database/model/migrations.jsapp/containers/InAppNotification/index.tsxapp/definitions/IRoom.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/lib/database/model/Subscription.jsapp/definitions/ISubscription.tsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/database/schema/app.jsapp/lib/methods/helpers/playNotificationSound.tsapp/lib/database/model/migrations.jsapp/containers/InAppNotification/index.tsxapp/definitions/IRoom.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/database/model/Subscription.jsapp/definitions/ISubscription.tsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/database/schema/app.jsapp/lib/methods/helpers/playNotificationSound.tsapp/lib/database/model/migrations.jsapp/containers/InAppNotification/index.tsxapp/definitions/IRoom.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/definitions/ISubscription.tsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/methods/helpers/playNotificationSound.tsapp/containers/InAppNotification/index.tsxapp/definitions/IRoom.ts
app/definitions/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place shared TypeScript type definitions in 'app/definitions/' directory
Files:
app/definitions/ISubscription.tsapp/definitions/IRoom.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/InAppNotification/index.tsx
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/definitions/ISubscription.tsapp/lib/methods/helpers/mergeSubscriptionsRooms.tsapp/lib/methods/helpers/playNotificationSound.tsapp/containers/InAppNotification/index.tsxapp/definitions/IRoom.ts
🔇 Additional comments (5)
app/definitions/IRoom.ts (1)
65-66: LGTM!Also applies to: 243-244, 254-254
app/definitions/ISubscription.ts (1)
118-118: LGTM!Also applies to: 183-183
app/lib/database/schema/app.js (1)
4-4: LGTM!Also applies to: 77-78
app/lib/database/model/Subscription.js (1)
164-164: LGTM!Also applies to: 236-237
app/lib/database/model/migrations.js (1)
348-356: LGTM!
Signed-off-by: psrsingh <psr.singh336@gmail.com>
2 participants
Summary
Fixes #7192 by enabling room-specific in-app notification sound playback using
audioNotificationValue.Changes
audioNotificationValuein local subscription storageexpo-avaudioNotificationsValueNotes
The mobile repository currently includes limited bundled notification sound assets, so existing bundled sounds are reused to validate room-specific playback behavior.
Testing
Tested on Android:
Summary by CodeRabbit