RE1-T88 Looking at using FCM to forward APNS messages with Novu #264
Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
WalkthroughUpdate to NovuProvider: Changes
Sequence DiagramsequenceDiagram
participant Caller
participant NovuProvider
participant PayloadBuilder
participant NovuAPI
Caller->>NovuProvider: UpdateSubscriberApns(id, token, apnsId?, fcmId?)
alt apnsId provided
NovuProvider->>PayloadBuilder: build credential { providerId: "apns", integrationIdentifier: apnsId }
else fcmId provided
NovuProvider->>PayloadBuilder: build credential { providerId: "fcm", integrationIdentifier: fcmId }
else neither provided
NovuProvider-->>Caller: return false (no payload)
end
PayloadBuilder-->>NovuProvider: jsonContent
NovuProvider->>NovuAPI: POST /subscribers/update with jsonContent
NovuAPI-->>NovuProvider: Response (success/failure)
NovuProvider-->>Caller: Task<bool> result
Note right of NovuProvider: SendNotification now injects APNS fields under overrides.fcm.android (badge, sound, type, category, eventCode)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Providers/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code graph analysis (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (3)
Core/Resgrid.Services/PushService.cs (5)
Task(31-48)Task(50-55)Task(57-75)Task(77-82)Task(89-120)Core/Resgrid.Model/Providers/Models/INovuProvider.cs (11)
Task(20-20)Task(31-31)Task(40-40)Task(49-49)Task(58-58)Task(67-67)Task(82-83)Task(90-90)Task(92-92)Task(94-94)Task(96-96)Core/Resgrid.Config/ChatConfig.cs (1)
ChatConfig(3-21)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Providers/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code graph analysis (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (3)
Core/Resgrid.Services/PushService.cs (5)
Task(31-48)Task(50-55)Task(57-75)Task(77-82)Task(89-120)Core/Resgrid.Model/Providers/Models/INovuProvider.cs (11)
Task(20-20)Task(31-31)Task(40-40)Task(49-49)Task(58-58)Task(67-67)Task(82-83)Task(90-90)Task(92-92)Task(94-94)Task(96-96)Core/Resgrid.Config/ChatConfig.cs (1)
ChatConfig(3-21)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
| public async Task<bool> UpdateUnitSubscriberApns(int unitId, string code, string token) | ||
| { | ||
| return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApnsProviderId); | ||
| //return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApnsProviderId); | ||
| return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, null, ChatConfig.NovuUnitFcmProviderId); | ||
| } |
There was a problem hiding this comment.
iOS unit registrations are now pushed into the wrong provider
UpdateUnitSubscriberApns no longer stores iOS tokens under the APNS integration and instead pushes them into the FCM provider. APNS device tokens are 64‑char hex strings that must be registered against an APNS integration; feeding them to FCM results in INVALID_ARGUMENT/UNREGISTERED responses because FCM expects its own registration tokens (longer base64 strings) and rejects APNS tokens outright. The net effect is that unit notifications on iOS will stop delivering. Please keep iOS registrations on the APNS integration (as before) or ensure the caller passes real FCM registration tokens before switching providers. (docs.novu.co)
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Messaging/NovuProvider.cs around lines 232-236,
UpdateUnitSubscriberApns is registering iOS APNS tokens against the FCM
provider; change the call to register tokens with the APNS integration instead.
Replace the current call that passes the FCM provider id with the APNS provider
id (e.g. use ChatConfig.NovuUnitApnsProviderId) so iOS 64‑char hex device tokens
are stored under APNS, not FCM.
| apns = new | ||
| { | ||
| badge = count, | ||
| sound = new | ||
| { | ||
| name = sound, | ||
| critical = channelName == "calls" ? 1 : 0, | ||
| volume = 1.0f | ||
| }, | ||
| type = type, | ||
| category = channelName, | ||
| eventCode = eventCode, | ||
| }, |
There was a problem hiding this comment.
Fix the FCM APNS override payload structure
The new overrides.fcm.apns block sets badge, sound, etc. directly on the APNS object, but FCM expects an ApnsConfig with payload.aps.* fields. With the current shape, FCM silently drops these keys, so iOS devices never receive the badge or custom sound. Please wrap the fields under payload.aps (and keep headers if needed) to match the documented structure.
- apns = new
- {
- badge = count,
- sound = new
- {
- name = sound,
- critical = channelName == "calls" ? 1 : 0,
- volume = 1.0f
- },
- type = type,
- category = channelName,
- eventCode = eventCode,
- },
+ apns = new
+ {
+ payload = new
+ {
+ aps = new
+ {
+ badge = count,
+ sound = new
+ {
+ name = sound,
+ critical = channelName == "calls" ? 1 : 0,
+ volume = 1.0f
+ },
+ category = channelName,
+ @event = eventCode,
+ customType = type
+ }
+ }
+ },(Adjust the exact keys you need under aps; Apple expects them inside payload.aps.) (novu-preview.mintlify.app)
📝 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.
| apns = new | |
| { | |
| badge = count, | |
| sound = new | |
| { | |
| name = sound, | |
| critical = channelName == "calls" ? 1 : 0, | |
| volume = 1.0f | |
| }, | |
| type = type, | |
| category = channelName, | |
| eventCode = eventCode, | |
| }, | |
| apns = new | |
| { | |
| payload = new | |
| { | |
| aps = new | |
| { | |
| badge = count, | |
| sound = new | |
| { | |
| name = sound, | |
| critical = channelName == "calls" ? 1 : 0, | |
| volume = 1.0f | |
| }, | |
| category = channelName, | |
| @event = eventCode, | |
| customType = type | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Messaging/NovuProvider.cs around lines 287 to
299, the APNS fields (badge, sound, type, category, eventCode) are being placed
directly under overrides.fcm.apns which FCM will ignore; wrap these keys under
payload.aps (e.g. payload: { aps: { badge: ..., sound: ..., ... } }) so the APNS
payload matches FCM/Apple expectations, and preserve any headers (apns.headers)
if required; adjust the object shape accordingly so badge and custom sound are
delivered to iOS devices.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
136-194: Method name misleading: Consider refactoring for clarity.The method is named
UpdateSubscriberApnsbut now handles both APNS and FCM registration paths. This violates single responsibility and makes the code harder to understand. The conditional branching based on which provider ID is present adds cognitive load.Consider one of these approaches:
- Extract into two separate methods:
UpdateSubscriberApnsandUpdateSubscriberFcm(note: a separateUpdateSubscriberFcmalready exists at line 100)- Rename to
UpdateSubscriberPushCredentialsto reflect its dual purpose- Keep the existing
UpdateSubscriberFcm(line 100) for FCM-only paths and restrictUpdateSubscriberApnsto APNS-onlyAs per coding guidelines: "Separate state from behavior" and "Use meaningful, descriptive names for types, methods, and parameters."
♻️ Duplicate comments (2)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (2)
235-236: Critical: iOS unit tokens incorrectly routed to FCM provider.Line 236 registers iOS device tokens with the FCM integration (
ChatConfig.NovuUnitFcmProviderId) instead of APNS. This is a critical issue because:
- APNS device tokens are 64-character hex strings that must be registered with an APNS integration
- FCM expects its own registration tokens (longer base64 format) and will reject APNS tokens with
INVALID_ARGUMENTorUNREGISTEREDerrors- From
PushService.cslines 56-74: iOS platform (PlatformType == Platforms.iOS) callsUpdateUnitSubscriberApns, which now routes to FCM- Result: iOS unit push notifications will fail to deliver
The commented-out line 235 shows the correct implementation using
ChatConfig.NovuUnitApnsProviderId.- //return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApnsProviderId); - return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, null, ChatConfig.NovuUnitFcmProviderId); + return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApnsProviderId, null);Note: If the PR objective is to test FCM passthrough for APNS, the caller must provide FCM registration tokens (not APNS tokens) and the platform check in
PushService.RegisterUnitmust be updated accordingly.
288-316: Fix FCM APNS override: Remove duplicate direct fields and correct payload structure.The current structure includes APNS fields both directly on the
apnsobject (lines 290-299) and nested underpayload.aps(lines 302-314). FCM will silently ignore the direct fields. Additionally, custom fields likeeventCodeandcustomTypeinsideaps(lines 312-313) are non-standard and may be dropped by Apple.Issues:
- Duplicate field definitions (badge, sound appear twice)
- Direct fields (lines 290-299) are not part of FCM's APNS config schema
- Custom fields should be outside the
apsobject for Apple to deliver them as custom dataapns = new { - badge = count, - sound = new - { - name = sound, - critical = channelName == "calls" ? 1 : 0, - volume = 1.0f - }, - type = type, - category = channelName, - eventCode = eventCode, payload = new { aps = new { badge = count, sound = new { name = sound, critical = channelName == "calls" ? 1 : 0, volume = 1.0f }, category = channelName, - eventCode = eventCode, - customType = type }, + eventCode = eventCode, + type = type }, },This moves custom fields (
eventCode,type) outsideapswhere they belong and removes the duplicate direct fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Providers/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code graph analysis (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (3)
Core/Resgrid.Services/PushService.cs (5)
Task(31-48)Task(50-55)Task(57-75)Task(77-82)Task(89-120)Core/Resgrid.Model/Providers/Models/INovuProvider.cs (11)
Task(20-20)Task(31-31)Task(40-40)Task(49-49)Task(58-58)Task(67-67)Task(82-83)Task(90-90)Task(92-92)Task(94-94)Task(96-96)Core/Resgrid.Config/ChatConfig.cs (1)
ChatConfig(3-21)
⏰ 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). (5)
- GitHub Check: build
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (2)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (2)
178-181: Guard logic correctly implemented.The early return when neither
apnsIdnorfcmIdis provided properly prevents sending an empty payload to Novu, addressing the previous review concern.
225-225: Correct routing for iOS user registrations.Passing
nullforfcmIdensures iOS user tokens are registered to the APNS integration viaChatConfig.NovuResponderApnsProviderId, which is the correct behavior.
| using Resgrid.Model; | ||
| using Resgrid.Model.Providers; | ||
| using Resgrid.Providers.Bus.Models; | ||
| using SharpCompress.Common; |
There was a problem hiding this comment.
Unused import: Remove if not needed.
The SharpCompress.Common namespace doesn't appear to be used anywhere in this file. Please remove it unless it's required for functionality outside the shown code.
-using SharpCompress.Common;📝 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.
| using SharpCompress.Common; | |
| // Line 10 and the using SharpCompress.Common; statement removed |
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Messaging/NovuProvider.cs around line 10, the
using directive for SharpCompress.Common is unused; remove the 'using
SharpCompress.Common;' line to clean up imports and ensure no references to that
namespace remain in the file (if any are later added, re-add the using where
needed).
| apns = new Dictionary<string, object> | ||
| { | ||
| name = sound, | ||
| critical = channelName == "calls" ? 1 : 0, | ||
| volume = 1.0f | ||
| ["badge"] = count, | ||
| ["sound"] = new | ||
| { | ||
| name = sound, | ||
| critical = channelName == "calls" ? 1 : 0, | ||
| volume = 1.0f | ||
| }, | ||
| ["type"] = type, | ||
| ["category"] = channelName, | ||
| ["eventCode"] = eventCode, | ||
| ["gcm.message_id"] = "123" | ||
| }, |
There was a problem hiding this comment.
Remove or clarify gcm.message_id in APNS override.
Line 330 adds ["gcm.message_id"] = "123" to the APNS override dictionary. This is problematic:
gcm.message_idis a GCM (Google Cloud Messaging, FCM's predecessor) concept and doesn't belong in APNS payloads- The hardcoded value
"123"suggests placeholder code - APNS has no concept of GCM message IDs and will likely ignore or reject this field
If this field is required for some FCM-APNS passthrough scenario, please:
- Document why it's needed
- Generate a meaningful value instead of hardcoding
"123" - Verify that Novu/APNS doesn't reject this unexpected field
Otherwise, remove it:
["type"] = type,
["category"] = channelName,
["eventCode"] = eventCode,
- ["gcm.message_id"] = "123"
},📝 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.
| apns = new Dictionary<string, object> | |
| { | |
| name = sound, | |
| critical = channelName == "calls" ? 1 : 0, | |
| volume = 1.0f | |
| ["badge"] = count, | |
| ["sound"] = new | |
| { | |
| name = sound, | |
| critical = channelName == "calls" ? 1 : 0, | |
| volume = 1.0f | |
| }, | |
| ["type"] = type, | |
| ["category"] = channelName, | |
| ["eventCode"] = eventCode, | |
| ["gcm.message_id"] = "123" | |
| }, | |
| apns = new Dictionary<string, object> | |
| { | |
| ["badge"] = count, | |
| ["sound"] = new | |
| { | |
| name = sound, | |
| critical = channelName == "calls" ? 1 : 0, | |
| volume = 1.0f | |
| }, | |
| ["type"] = type, | |
| ["category"] = channelName, | |
| ["eventCode"] = eventCode, | |
| }, |
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Messaging/NovuProvider.cs around lines 318 to
331, the APNS override currently contains a hardcoded GCM field
["gcm.message_id"] = "123", which is inappropriate for APNS; remove that key
from the APNS dictionary, or if you truly need to pass an FCM/Android message
id, move it to the FCM/GCM-specific payload branch (not APNS), generate a
meaningful unique id (e.g., guid or provider message id) instead of "123", and
add a short comment documenting why and where that field is used and a check to
ensure Novu/APNS won’t reject it.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit