Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds an Android-only "Modern Notification Sounds" toggle, backed by a new MMKV-persisted preference hook ( ChangesModern Notification Sounds
Sequence Diagram(s)sequenceDiagram
participant ModernNotificationSoundsItem
participant useModernNotificationSounds
participant pushNotificationService
participant NotificationsAPI
ModernNotificationSoundsItem->>useModernNotificationSounds: setModernNotificationSoundsEnabled(false)
useModernNotificationSounds->>useModernNotificationSounds: persist boolean via MMKV
ModernNotificationSoundsItem->>pushNotificationService: refreshNotificationChannelSounds()
pushNotificationService->>pushNotificationService: getModernNotificationSoundsEnabled()
loop for each standard channel
pushNotificationService->>NotificationsAPI: deleteNotificationChannelAsync(channelId)
pushNotificationService->>NotificationsAPI: setNotificationChannelAsync(channelId, selectedSound)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
src/components/settings/modern-notification-sounds-item.tsx (1)
34-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwitch lacks an accessible name for screen readers.
The
Switchhas noaccessibilityLabel/accessibilityRoletied to the setting's purpose; screen-reader users only get the visualTextsibling, which isn't programmatically associated with the control.♿ Suggested fix
- <Switch size="md" value={isModernNotificationSoundsEnabled} onValueChange={handleToggle} /> + <Switch + size="md" + value={isModernNotificationSoundsEnabled} + onValueChange={handleToggle} + accessibilityLabel={t('settings.modern_notification_sounds')} + />As per coding guidelines, "Ensure the app is accessible, following WCAG guidelines for mobile applications."
🤖 Prompt for 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. In `@src/components/settings/modern-notification-sounds-item.tsx` around lines 34 - 43, The `Switch` in `ModernNotificationSoundsItem` is missing a programmatic accessible name, so screen readers cannot tell what setting it controls. Update the `Switch` component to expose the setting purpose directly by adding an appropriate accessibility label and role, using the existing translated text from `t('settings.modern_notification_sounds')` as the name. Keep the change localized to `ModernNotificationSoundsItem` and ensure the label remains associated with the toggle even when the surrounding `Text` is not read together.Source: Coding guidelines
src/components/settings/__tests__/modern-notification-sounds-item.test.tsx (1)
31-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock component props typed as
any.The
Switch,Text,View, andVStackmocks all type their props asany. As per coding guidelines, "Avoid usingany; use precise types" applies to**/*.{ts,tsx}files.🔧 Suggested fix
- Switch: ({ value, onValueChange }: any) => { + Switch: ({ value, onValueChange }: { value: boolean; onValueChange: (v: boolean) => void }) => {🤖 Prompt for 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. In `@src/components/settings/__tests__/modern-notification-sounds-item.test.tsx` around lines 31 - 57, The mocked React components in the test file currently type their props as any, which violates the no-any guideline for TypeScript tests. Update the mock component signatures for Switch, Text, View, and VStack to use precise prop types from their real components or minimal inline interfaces for the specific props they consume, and keep the mock behavior in the same jest.mock blocks so the test references remain unchanged.Source: Coding guidelines
🤖 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 `@src/services/push-notification.ts`:
- Around line 55-62: The notification channel labels and descriptions in
NOTIFICATION_CHANNELS are hardcoded English text, so update the channel config
in push-notification.ts to use i18n via t() from react-i18next instead of
literals. Locate the NOTIFICATION_CHANNELS array and replace each user-visible
name/description with translation keys sourced from the locale files in
src/translations, keeping the channel ids and sound values unchanged.
- Around line 107-114: The standard notification channels in
push-notification.ts are only being updated via createNotificationChannel, so
existing Android channels keep their old sound values when
getModernNotificationSoundsEnabled() first defaults to true. Add a one-time
migration in the same flow that detects first-seen modern sounds and
deletes/recreates the standard channels (using the existing channel loop and
createNotificationChannel helper) before proceeding, so upgraded installs
actually receive the new sound configuration.
- Around line 145-162: `refreshNotificationChannelSounds()` currently catches
channel API failures, logs them, and then resolves, which hides refresh errors
from callers. Update the try/catch in `push-notification.ts` so failures are not
swallowed: after `logger.error(...)`, re-throw the caught error or return an
explicit failure result that the caller can inspect. Keep the existing
`useModernSounds`, `Notifications.deleteNotificationChannelAsync`, and
`createNotificationChannel` flow intact, but ensure the settings toggle’s caller
can detect and handle refresh failures.
---
Nitpick comments:
In `@src/components/settings/__tests__/modern-notification-sounds-item.test.tsx`:
- Around line 31-57: The mocked React components in the test file currently type
their props as any, which violates the no-any guideline for TypeScript tests.
Update the mock component signatures for Switch, Text, View, and VStack to use
precise prop types from their real components or minimal inline interfaces for
the specific props they consume, and keep the mock behavior in the same
jest.mock blocks so the test references remain unchanged.
In `@src/components/settings/modern-notification-sounds-item.tsx`:
- Around line 34-43: The `Switch` in `ModernNotificationSoundsItem` is missing a
programmatic accessible name, so screen readers cannot tell what setting it
controls. Update the `Switch` component to expose the setting purpose directly
by adding an appropriate accessibility label and role, using the existing
translated text from `t('settings.modern_notification_sounds')` as the name.
Keep the change localized to `ModernNotificationSoundsItem` and ensure the label
remains associated with the toggle even when the surrounding `Text` is not read
together.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: baf2ef0d-171b-4b1c-a0a3-a0c27c6e24a1
⛔ Files ignored due to path filters (20)
assets/audio/modernavailabilityalert.wavis excluded by!**/*.wavassets/audio/moderncalendar.wavis excluded by!**/*.wavassets/audio/moderncallclosed.wavis excluded by!**/*.wavassets/audio/moderncallemergency.wavis excluded by!**/*.wavassets/audio/moderncallhigh.wavis excluded by!**/*.wavassets/audio/moderncalllow.wavis excluded by!**/*.wavassets/audio/moderncallmedium.wavis excluded by!**/*.wavassets/audio/moderncallupdated.wavis excluded by!**/*.wavassets/audio/modernchat.wavis excluded by!**/*.wavassets/audio/modernmessage.wavis excluded by!**/*.wavassets/audio/modernnotification.wavis excluded by!**/*.wavassets/audio/modernpersonnelstatus.wavis excluded by!**/*.wavassets/audio/modernresourceorder.wavis excluded by!**/*.wavassets/audio/modernshift.wavis excluded by!**/*.wavassets/audio/modernstaffing.wavis excluded by!**/*.wavassets/audio/moderntraining.wavis excluded by!**/*.wavassets/audio/moderntroublealert.wavis excluded by!**/*.wavassets/audio/modernunitnotice.wavis excluded by!**/*.wavassets/audio/modernunitstatus.wavis excluded by!**/*.wavassets/audio/modernweatheralert.wavis excluded by!**/*.wav
📒 Files selected for processing (20)
app.config.tssrc/app/(app)/__tests__/settings.test.tsxsrc/app/(app)/settings.tsxsrc/components/settings/__tests__/modern-notification-sounds-item.test.tsxsrc/components/settings/modern-notification-sounds-item.tsxsrc/lib/hooks/__tests__/use-modern-notification-sounds.test.tsxsrc/lib/hooks/index.tsxsrc/lib/hooks/use-modern-notification-sounds.tsxsrc/services/__tests__/push-notification-channels.test.tssrc/services/__tests__/push-notification.test.tssrc/services/push-notification.tssrc/translations/ar.jsonsrc/translations/de.jsonsrc/translations/en.jsonsrc/translations/es.jsonsrc/translations/fr.jsonsrc/translations/it.jsonsrc/translations/pl.jsonsrc/translations/sv.jsonsrc/translations/uk.json
| const NOTIFICATION_CHANNELS: NotificationChannelConfig[] = [ | ||
| { id: 'calls', name: 'Generic Call', description: 'Generic Call', modernSound: 'modernnotification' }, | ||
| { id: '0', name: 'Emergency Call', description: 'Emergency Call', modernSound: 'moderncallemergency', classicSound: 'callemergency' }, | ||
| { id: '1', name: 'High Call', description: 'High Call', modernSound: 'moderncallhigh', classicSound: 'callhigh' }, | ||
| { id: '2', name: 'Medium Call', description: 'Medium Call', modernSound: 'moderncallmedium', classicSound: 'callmedium' }, | ||
| { id: '3', name: 'Low Call', description: 'Low Call', modernSound: 'moderncalllow', classicSound: 'calllow' }, | ||
| { id: 'notif', name: 'Notification', description: 'Notifications', modernSound: 'modernnotification', vibration: false }, | ||
| { id: 'message', name: 'Message', description: 'Messages', modernSound: 'modernmessage', vibration: false }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localize the channel labels and descriptions.
These strings show up in Android’s notification settings, so hardcoded English here bypasses the locale files added in this PR. Please source them from i18n instead of embedding literals. As per coding guidelines, "All user-visible text must be wrapped in t() from react-i18next" and "Ensure all text is wrapped in t() from react-i18next for translations with the dictionary files stored in src/translations".
🤖 Prompt for 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.
In `@src/services/push-notification.ts` around lines 55 - 62, The notification
channel labels and descriptions in NOTIFICATION_CHANNELS are hardcoded English
text, so update the channel config in push-notification.ts to use i18n via t()
from react-i18next instead of literals. Locate the NOTIFICATION_CHANNELS array
and replace each user-visible name/description with translation keys sourced
from the locale files in src/translations, keeping the channel ids and sound
values unchanged.
Source: Coding guidelines
| try { | ||
| const useModernSounds = getModernNotificationSoundsEnabled(); | ||
|
|
||
| for (const channel of NOTIFICATION_CHANNELS) { | ||
| await Notifications.deleteNotificationChannelAsync(channel.id); | ||
| const sound = useModernSounds ? channel.modernSound : channel.classicSound; | ||
| await this.createNotificationChannel(channel.id, channel.name, channel.description, sound, channel.vibration ?? true); | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: 'Android notification channel sounds refreshed', | ||
| context: { useModernSounds }, | ||
| }); | ||
| } catch (error) { | ||
| logger.error({ | ||
| message: 'Error refreshing Android notification channel sounds', | ||
| context: { error }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t swallow refresh failures.
refreshNotificationChannelSounds() logs and resolves on error. The settings toggle persists the preference before calling this method, so any channel API failure leaves MMKV and the actual Android channel sounds out of sync with no way to revert or notify the user. Re-throw here (or return an explicit failure result) so the caller can handle it. As per coding guidelines, "Handle errors gracefully and provide user feedback".
Minimal service-side change
} catch (error) {
logger.error({
message: 'Error refreshing Android notification channel sounds',
context: { error },
});
+ throw error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const useModernSounds = getModernNotificationSoundsEnabled(); | |
| for (const channel of NOTIFICATION_CHANNELS) { | |
| await Notifications.deleteNotificationChannelAsync(channel.id); | |
| const sound = useModernSounds ? channel.modernSound : channel.classicSound; | |
| await this.createNotificationChannel(channel.id, channel.name, channel.description, sound, channel.vibration ?? true); | |
| } | |
| logger.info({ | |
| message: 'Android notification channel sounds refreshed', | |
| context: { useModernSounds }, | |
| }); | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Error refreshing Android notification channel sounds', | |
| context: { error }, | |
| }); | |
| try { | |
| const useModernSounds = getModernNotificationSoundsEnabled(); | |
| for (const channel of NOTIFICATION_CHANNELS) { | |
| await Notifications.deleteNotificationChannelAsync(channel.id); | |
| const sound = useModernSounds ? channel.modernSound : channel.classicSound; | |
| await this.createNotificationChannel(channel.id, channel.name, channel.description, sound, channel.vibration ?? true); | |
| } | |
| logger.info({ | |
| message: 'Android notification channel sounds refreshed', | |
| context: { useModernSounds }, | |
| }); | |
| } catch (error) { | |
| logger.error({ | |
| message: 'Error refreshing Android notification channel sounds', | |
| context: { error }, | |
| }); | |
| throw error; | |
| } |
🤖 Prompt for 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.
In `@src/services/push-notification.ts` around lines 145 - 162,
`refreshNotificationChannelSounds()` currently catches channel API failures,
logs them, and then resolves, which hides refresh errors from callers. Update
the try/catch in `push-notification.ts` so failures are not swallowed: after
`logger.error(...)`, re-throw the caught error or return an explicit failure
result that the caller can inspect. Keep the existing `useModernSounds`,
`Notifications.deleteNotificationChannelAsync`, and `createNotificationChannel`
flow intact, but ensure the settings toggle’s caller can detect and handle
refresh failures.
Source: Coding guidelines
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Pull Request Description
This PR introduces a "Modern Notification Sounds" preference for Android users, allowing them to toggle between a new set of modern push notification sounds and the classic sounds.
Key changes:
New modern sound assets: 20 new audio files (e.g.,
moderncallemergency.wav,moderncallhigh.wav,modernnotification.wav,modernmessage.wav) are now bundled with the app.User setting: A new toggle is added to the Settings screen (Android only) that lets users enable or disable modern notification sounds. The preference is persisted locally via MMKV and defaults to enabled.
Push notification channel integration: The push notification service was refactored to use a declarative channel configuration. When modern sounds are enabled, each standard notification channel plays its corresponding modern sound—including channels (
calls,notif,message) that were previously silent. When disabled, channels revert to their classic sounds (or silence for channels that originally had none).Channel refresh mechanism: Because Android locks a notification channel's sound at creation time, a new
refreshNotificationChannelSounds()method deletes and recreates the affected channels whenever the user toggles the preference, ensuring the new sound takes effect immediately.