RU-T39 Trying to fix foreground push notification issue. #184
Conversation
WalkthroughThis PR migrates push notification handling from expo-notifications to a Notifee/Firebase-based architecture. It introduces two new Expo config plugins to auto-configure iOS entitlements and deploy notification sounds, refactors the notification service with iOS-specific category setup and Notifee event listeners for foreground/tap handling, adds documentation for iOS fixes, and removes unused dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant iOS as iOS Native
participant Delegate as AppDelegate<br/>UNUserNotificationCenter
participant Notifee as Notifee
participant RN as React Native<br/>(push-notification.ts)
participant Modal as Notification Modal
Note over iOS,Modal: Foreground Notification Delivery
iOS->>Delegate: Remote notification arrives (app in foreground)
Delegate->>Delegate: userNotificationCenter(willPresent)
Delegate->>Notifee: Display via Notifee<br/>(with sound, badge, banner)
Notifee->>RN: Foreground event listener triggered
RN->>RN: Extract eventCode, title, body<br/>Build modal data
RN->>Modal: Show notification modal<br/>(after optional delay)
Modal-->>Modal: User views/dismisses
Note over iOS,Modal: Tap Handling (Foreground)
User->>Notifee: Tap notification
Notifee->>RN: Background event listener triggered
RN->>RN: Delayed handler (300ms+)<br/>Extract eventCode
RN->>Modal: Show notification modal
sequenceDiagram
participant App as App Init
participant RN as React Native<br/>(push-notification.ts)
participant Firebase as Firebase Messaging
participant Notifee as Notifee
participant Backend as Backend API
Note over App,Backend: Permission & Registration Flow (iOS)
App->>RN: initialize()
RN->>RN: setupIOSNotificationCategories()
RN->>RN: Register Notifee foreground/background listeners
App->>RN: registerForPushNotifications()
RN->>RN: Validate unitId
RN->>Firebase: Check/request permission<br/>(with critical alerts enabled)
Firebase-->>RN: Permission granted/denied
alt Permission Denied
RN->>RN: Log and abort
else Permission Granted
RN->>Notifee: Request notification permission
Notifee-->>RN: Permission result
RN->>Firebase: Retrieve FCM token
Firebase-->>RN: Token
RN->>Backend: Register device with token
Backend-->>RN: Registration confirmed
RN->>RN: Store token, log success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*📄 CodeRabbit inference engine (.cursorrules)
Files:
🧬 Code graph analysis (1)src/services/push-notification.ts (2)
⏰ 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). (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. 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: 4
🧹 Nitpick comments (5)
expo-env.d.ts (1)
3-3: Verify this file is in .gitignore.The comment indicates this file should be in git ignore, but it appears to be tracked in this PR. Consider adding
expo-env.d.tsto.gitignoreto prevent merge conflicts from environment-specific type generation.scripts/add-sounds-to-xcode.js (1)
81-84: Simplify build phase lookup.The nested
findwithpbxResourcesBuildPhaseSection()lookup is verbose. Consider extracting to a helper function for clarity.Apply this refactor for improved readability:
+ const findResourcesBuildPhase = (target, project) => { + return target.pbxNativeTarget.buildPhases.find((phase) => { + const phaseObj = project.pbxResourcesBuildPhaseSection()[phase.value]; + return phaseObj && phaseObj.isa === 'PBXResourcesBuildPhase'; + }); + }; + if (fileRef) { // Add to resources build phase - const resourcesBuildPhase = target.pbxNativeTarget.buildPhases.find((phase) => { - const phaseObj = project.pbxResourcesBuildPhaseSection()[phase.value]; - return phaseObj && phaseObj.isa === 'PBXResourcesBuildPhase'; - }); + const resourcesBuildPhase = findResourcesBuildPhase(target, project);plugins/withForegroundNotifications.js (3)
31-38: String replacements are fragile.The simple string replacement approach for imports and class declarations will fail if:
- The import order changes
- The class declaration has different formatting
- Multiple classes are defined
Consider using more robust patterns or AST manipulation. For example:
- if (!contents.includes('import UserNotifications')) { - contents = contents.replace(/import ReactAppDependencyProvider/, 'import ReactAppDependencyProvider\nimport UserNotifications'); + if (!contents.includes('import UserNotifications')) { + // Add import at the top of the file after existing imports + contents = contents.replace( + /(import\s+\w+\s*\n)+/, + '$&import UserNotifications\n' + ); }Alternatively, document the assumptions about AppDelegate structure in comments.
43-52: Firebase config pattern may not match in all scenarios.The regex pattern assumes a specific comment format from
@react-native-firebase/app. If the Firebase plugin changes its comment format or placement, this injection will fail silently.Add error handling or validation:
const firebaseConfigPattern = /FirebaseApp\.configure\(\)\n\/\/ @generated end @react-native-firebase\/app-didFinishLaunchingWithOptions/; + + if (!firebaseConfigPattern.test(contents)) { + throw new Error( + 'Could not find Firebase configuration pattern in AppDelegate. ' + + 'The AppDelegate structure may have changed. Please update the plugin.' + ); + } contents = contents.replace( firebaseConfigPattern,
59-90: Consider extracting delegate method templates to separate constants.The large template string embedded in the regex replacement makes the code harder to read and maintain.
Extract to constants at the top of the file:
+const NOTIFICATION_CENTER_DELEGATE_METHODS = ` + // Handle foreground notifications - tell iOS to show them + public func userNotificationCenter( + _ center: UNUserNotificationCenter, + willPresent notification: UNNotification, + withCompletionHandler completionHandler: @escaping (UNNotificationPresentationOptions) -> Void + ) { + // Show notification with alert, sound, and badge even when app is in foreground + if #available(iOS 14.0, *) { + completionHandler([.banner, .sound, .badge]) + } else { + completionHandler([.alert, .sound, .badge]) + } + } + + // Handle notification tap - when user taps on a notification + public func userNotificationCenter( + _ center: UNUserNotificationCenter, + didReceive response: UNNotificationResponse, + withCompletionHandler completionHandler: @escaping () -> Void + ) { + // Forward the notification response to React Native + // When using Notifee (v7+), it will handle notification taps automatically + // This method still needs to be implemented to receive the notification response + // The response will be handled by Notifee's onBackgroundEvent/onForegroundEvent + completionHandler() + } +`; if (!contents.includes('userNotificationCenter(_ center: UNUserNotificationCenter')) { const linkingApiPattern = /(\s+)(\/\/ Linking API)/; - - const delegateMethod = ` - // Handle foreground notifications - tell iOS to show them - ... -$1$2`; - - contents = contents.replace(linkingApiPattern, delegateMethod); + contents = contents.replace(linkingApiPattern, `\n${NOTIFICATION_CENTER_DELEGATE_METHODS}\n$1$2`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.DS_Storeis excluded by!**/.DS_Storeyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
app.config.ts(1 hunks)docs/ios-foreground-notifications-fix.md(1 hunks)docs/ios-push-notification-tap-fix.md(1 hunks)eas.json(1 hunks)expo-env.d.ts(1 hunks)package.json(0 hunks)plugins/withForegroundNotifications.js(1 hunks)plugins/withNotificationSounds.js(1 hunks)scripts/add-sounds-to-xcode.js(1 hunks)src/services/__tests__/push-notification.test.ts(2 hunks)src/services/push-notification.ts(7 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*
📄 CodeRabbit inference engine (.cursorrules)
Directory and file names should be lowercase and hyphenated (e.g., user-profile, chat-screen)
Files:
docs/ios-push-notification-tap-fix.mddocs/ios-foreground-notifications-fix.mdapp.config.tssrc/services/push-notification.tsplugins/withNotificationSounds.jsexpo-env.d.tsplugins/withForegroundNotifications.jssrc/services/__tests__/push-notification.test.tsscripts/add-sounds-to-xcode.jseas.json
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names (e.g., isFetchingData, handleUserInput)
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use Expo SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching and caching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
**/*.{ts,tsx}: Write concise, type-safe TypeScript code
Use camelCase for variable and function names
Use TypeScript for all components, favoring interfaces for props and state
Avoid using any; strive for precise types
Use React Navigation for handling navigation and deep linking
Handle errors gracefully and provide user feedback
Use Expo's SecureStore for sensitive data
Use zustand for state management
Use react-query for data fetching
Use react-i18next for internationalization
Use react-native-mmkv for local storage
Use axios for API requests
Files:
app.config.tssrc/services/push-notification.tsexpo-env.d.tssrc/services/__tests__/push-notification.test.ts
**/*.{test.ts,test.tsx,spec.ts,spec.tsx}
📄 CodeRabbit inference engine (.cursorrules)
Create and use Jest tests to validate all generated components
Files:
src/services/__tests__/push-notification.test.ts
**/__tests__/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Generate tests for all components, services, and logic; ensure tests run without errors
Files:
src/services/__tests__/push-notification.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create and use Jest tests for all generated components
Files:
src/services/__tests__/push-notification.test.ts
🧠 Learnings (4)
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
Repo: Resgrid/Unit PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps and navigation
Applied to files:
app.config.ts
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
Repo: Resgrid/Unit PR: 0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.tsx : Use rnmapbox/maps for maps or vehicle navigation
Applied to files:
app.config.ts
📚 Learning: 2025-08-12T03:34:25.992Z
Learnt from: CR
Repo: Resgrid/Unit PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-12T03:34:25.992Z
Learning: Applies to **/*.{ts,tsx} : Use Expo's SecureStore for sensitive data
Applied to files:
expo-env.d.ts
📚 Learning: 2025-08-12T03:33:40.238Z
Learnt from: CR
Repo: Resgrid/Unit PR: 0
File: .cursorrules:0-0
Timestamp: 2025-08-12T03:33:40.238Z
Learning: Applies to **/*.{ts,tsx} : Use Expo SecureStore for sensitive data
Applied to files:
expo-env.d.ts
🧬 Code graph analysis (3)
src/services/push-notification.ts (2)
src/lib/logging/index.tsx (2)
logger(80-80)error(74-76)src/stores/push-notification/store.ts (1)
usePushNotificationModalStore(32-104)
plugins/withNotificationSounds.js (1)
scripts/add-sounds-to-xcode.js (1)
soundFiles(11-56)
scripts/add-sounds-to-xcode.js (1)
plugins/withNotificationSounds.js (5)
require(1-1)require(2-2)require(3-3)soundFiles(9-54)fileName(71-71)
🪛 LanguageTool
docs/ios-push-notification-tap-fix.md
[style] ~94-~94: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “iOS”.
Context: ...ns displayed BY Notifee, not the native iOS system notifications. ## How It Works Now ##...
(ACRONYM_TAUTOLOGY)
[duplication] ~133-~133: Possible typo: you repeated a word.
Context: ... Have app in foreground 2. Receive push notification 3. Notification banner appears (via native iOS system) ...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~134-~134: This phrase is redundant (‘OS’ stands for ‘operating system’). Use simply “iOS”.
Context: ...Notification banner appears (via native iOS system) 4. Tap banner 5. Expected: Modal a...
(ACRONYM_TAUTOLOGY)
🪛 markdownlint-cli2 (0.18.1)
docs/ios-push-notification-tap-fix.md
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (7)
eas.json (1)
3-4: LGTM!The addition of
promptToConfigurePushNotifications: falseis appropriate given the migration to custom plugin-based push notification handling viawithForegroundNotifications.jsandwithNotificationSounds.js.scripts/add-sounds-to-xcode.js (1)
73-77: Review file reference configuration for correctness.The
addFileoptions specifysourceTree: '"<group>"'with escaped quotes. Verify this produces the correct Xcode project structure, as improperly quoted values can cause build issues.Consider logging the fileRef structure after creation to verify correct configuration:
const fileRef = project.addFile(fileName, null, { lastKnownFileType: 'audio.wav', defaultEncoding: 4, sourceTree: '"<group>"', }); + console.log('Created fileRef:', JSON.stringify(fileRef, null, 2));src/services/__tests__/push-notification.test.ts (1)
100-107: LGTM!The
mockDisplayNotificationis properly wired into the Notifee mock, providing test coverage for notification display functionality.plugins/withForegroundNotifications.js (1)
13-13: Verify hardcoded 'production' APS environment is correct for all builds.Setting
aps-environmentto'production'for all builds (including development) is unusual. Typically, development builds use'development'APS environment to work with development push certificates/tokens.This configuration may cause push notifications to fail in development builds if the backend sends notifications to the development APS endpoint. Verify that:
- The backend always sends notifications to the production APS endpoint
- Development push certificates are configured for production APS
- Or, this is intentional and documented
Consider making this environment-aware:
- entitlements['aps-environment'] = 'production'; + const env = process.env.APP_ENV || config.extra?.APP_ENV; + entitlements['aps-environment'] = (env === 'production' || env === 'internal') ? 'production' : 'development';docs/ios-push-notification-tap-fix.md (1)
1-191: Documentation is clear and comprehensive.The documentation thoroughly explains the tap handling issue, root causes, solution, and testing scenarios. The setTimeout-based approach with different delays for different app states (background vs killed) is pragmatic and well-justified.
Minor note: The delays (300ms, 1500ms) are empirically determined. Consider adding telemetry to track actual initialization times across devices to validate these values remain appropriate as the app evolves.
docs/ios-foreground-notifications-fix.md (1)
1-245: Documentation is thorough and well-structured.The documentation clearly explains the problem (missing APS entitlement, broken foreground/tap handling), the solution (config plugin + AppDelegate changes), and the benefits of the new approach. The flow diagrams and testing scenarios are particularly helpful.
Consider adding a troubleshooting section for common issues developers might encounter, such as:
- Plugin not applying changes → need to run
expo prebuild --clean- Notifications still not working → check provisioning profiles include push capability
- Sound not playing → verify sound files are in Xcode project
app.config.ts (1)
208-209: Plugin files verified and properly configured.Both
withForegroundNotifications.jsandwithNotificationSounds.jsexist in the plugins directory and have proper exports. The architectural improvement is sound.
scripts/add-sounds-to-xcode.js
Outdated
| const soundFiles = [ | ||
| 'notification.wav', | ||
| 'callclosed.wav', | ||
| 'callupdated.wav', | ||
| 'callemergency.wav', | ||
| 'callhigh.wav', | ||
| 'calllow.wav', | ||
| 'callmedium.wav', | ||
| 'newcall.wav', | ||
| 'newchat.wav', | ||
| 'newmessage.wav', | ||
| 'newshift.wav', | ||
| 'newtraining.wav', | ||
| 'personnelstaffingupdated.wav', | ||
| 'personnelstatusupdated.wav', | ||
| 'troublealert.wav', | ||
| 'unitnotice.wav', | ||
| 'unitstatusupdated.wav', | ||
| 'upcomingshift.wav', | ||
| 'upcomingtraining.wav', | ||
| 'c1.wav', | ||
| 'c2.wav', | ||
| 'c3.wav', | ||
| 'c4.wav', | ||
| 'c5.wav', | ||
| 'c6.wav', | ||
| 'c7.wav', | ||
| 'c8.wav', | ||
| 'c9.wav', | ||
| 'c10.wav', | ||
| 'c11.wav', | ||
| 'c12.wav', | ||
| 'c13.wav', | ||
| 'c14.wav', | ||
| 'c15.wav', | ||
| 'c16.wav', | ||
| 'c17.wav', | ||
| 'c18.wav', | ||
| 'c19.wav', | ||
| 'c20.wav', | ||
| 'c21.wav', | ||
| 'c22.wav', | ||
| 'c23.wav', | ||
| 'c24.wav', | ||
| 'c25.wav', | ||
| ]; |
There was a problem hiding this comment.
Critical: Filename mismatch with plugin configuration.
Line 17 defines 'calllow.wav' (two l's), but plugins/withNotificationSounds.js references 'assets/audio/callow.wav' (one l). This discrepancy will cause the script and plugin to reference different files, potentially breaking notification sounds.
Verify the correct filename and ensure consistency across:
- This script
plugins/withNotificationSounds.js- The actual asset files in
assets/audio/
#!/bin/bash
# Verify which filename exists in the assets directory
fd -e wav . assets/audio/ | rg -i "call.*low"🤖 Prompt for AI Agents
In scripts/add-sounds-to-xcode.js around lines 11 to 56 there is a filename
mismatch: this file lists 'calllow.wav' (with two l's) but
plugins/withNotificationSounds.js and/or the assets directory use 'callow.wav'
(one l), causing broken references; confirm the correct filename in assets/audio
(use the provided shell check), then make them consistent by either renaming the
asset to the canonical name or updating the entry in this array and the
reference in plugins/withNotificationSounds.js to the same exact filename (and
commit/rename in git if changing asset names).
| describe('iOS foreground notification display', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('should display notification on iOS when app is in foreground with emergency priority', () => { | ||
| const remoteMessage = createMockRemoteMessage({ | ||
| title: 'Emergency Call', | ||
| body: 'Structure fire at Main St', | ||
| data: { | ||
| eventCode: 'C:1234', | ||
| priority: '0', | ||
| }, | ||
| }); | ||
|
|
||
| // Since the service is already instantiated with iOS platform mock, | ||
| // we just need to verify the notification would be displayed | ||
| // The actual iOS-specific test needs to run on iOS platform | ||
| // For now, verify that the notification data structure is correct | ||
| expect(remoteMessage.notification).toBeDefined(); | ||
| expect(remoteMessage.notification.title).toBe('Emergency Call'); | ||
| expect(remoteMessage.data.priority).toBe('0'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test does not verify the intended behavior.
The test suite "iOS foreground notification display" only validates the structure of remoteMessage but never asserts that mockDisplayNotification is called or that the notification is actually displayed. The comment on line 406-408 acknowledges this gap.
Consider one of these approaches:
- If testing is blocked by platform constraints, rename the test to reflect what it actually tests:
- describe('iOS foreground notification display', () => {
+ describe('iOS foreground notification data structure', () => {
...
- it('should display notification on iOS when app is in foreground with emergency priority', () => {
+ it('should have valid notification structure for iOS foreground display with emergency priority', () => {- If testing is possible, actually test the display behavior by simulating the service's foreground message handler:
it('should display notification on iOS when app is in foreground with emergency priority', async () => {
// Import and initialize the service
const { pushNotificationService } = require('../push-notification');
await pushNotificationService.initialize();
const remoteMessage = createMockRemoteMessage({
title: 'Emergency Call',
body: 'Structure fire at Main St',
data: {
eventCode: 'C:1234',
priority: '0',
},
});
// Simulate the onMessage handler
const onMessageHandler = mockOnMessage.mock.calls[0][0];
await onMessageHandler(remoteMessage);
// Verify displayNotification was called on iOS
expect(mockDisplayNotification).toHaveBeenCalled();
});🤖 Prompt for AI Agents
In src/services/__tests__/push-notification.test.ts around lines 391 to 414, the
"iOS foreground notification display" test only checks the remoteMessage shape
and does not verify that the service actually displays a notification; update
the test to either rename it to reflect it's only validating the mock
remoteMessage shape (if platform constraints prevent running the iOS handler),
or preferably simulate the service foreground handler: import/initialize the
pushNotificationService, retrieve the registered onMessage handler from the mock
(mockOnMessage.mock.calls), invoke it with the created remoteMessage, await any
async initialization, and assert that mockDisplayNotification was called (and
optionally called with expected args); make sure to clear mocks in beforeEach
and use await where necessary so the assertion is reliable.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
Bug Fixes
Documentation
Chores