Fix APNs dropping Data/Url; add Badge/Sound/Silent/ImageUrl; add maxC… - #5
Merged
Conversation
…oncurrency throttle
There was a problem hiding this comment.
Pull request overview
This PR expands NotificationMessage to support richer cross-channel notification features (badge/sound/silent/image) and fixes APNs payload shaping so custom data isn’t dropped, while also adding an optional concurrency cap for large fan-outs.
Changes:
- Fix APNs payload structure to send
Data/Urlas top-level keys (Apple convention), plus supportBadge,Sound, andSilentbackground pushes. - Add
SilentandImageUrlhandling for FCM/WebPush/Webhook, and expose the new fields through the ASP.NET Core endpoints + README. - Add optional
maxConcurrencytoNotificationSender.SendAsyncwith tests to validate throttling behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/NotifyHub.Tests/NotificationSenderTests.cs | Adds coverage for the new maxConcurrency behavior. |
| tests/NotifyHub.Tests/FcmChannelTests.cs | Adds tests for ImageUrl and Silent (data-only) FCM payload behavior. |
| tests/NotifyHub.Tests/ApnsChannelTests.cs | Adds regression tests ensuring APNs includes Data/Url, plus badge/sound and background push headers. |
| src/NotifyHub/NotificationSender.cs | Introduces optional maxConcurrency throttling in the central send fan-out. |
| src/NotifyHub/NotificationMessage.cs | Extends the message contract with Badge, Sound, Silent, and ImageUrl plus updated XML docs. |
| src/NotifyHub/Channels/WebPushChannel.cs | Includes image and silent in the encrypted WebPush payload. |
| src/NotifyHub/Channels/WebhookChannel.cs | Extends the generic webhook payload to include the new fields. |
| src/NotifyHub/Channels/FcmChannel.cs | Implements Silent (data-only) and ImageUrl support in the FCM v1 payload. |
| src/NotifyHub/Channels/ApnsChannel.cs | Fixes APNs payload layout, adds badge/sound/silent handling and proper APNs headers for background pushes. |
| src/NotifyHub.AspNetCore/NotifyHubEndpoints.cs | Exposes new message fields and MaxConcurrency via the Minimal API send endpoint. |
| README.md | Documents new fields, APNs custom-key behavior, and the maxConcurrency option. |
Suppressed comments (2)
src/NotifyHub/NotificationMessage.cs:27
- The
Sounddocs say it's not applicable to Webhook, butWebhookChannelincludessoundin the generic payload (and README documents it). Update the XML docs so channel behavior is consistent with documentation.
/// <summary>Custom notification sound (APNs <c>aps.sound</c>). Defaults to the platform's
/// standard sound when left unset. Not applicable to WebPush/FCM/Webhook/Email.</summary>
public string? Sound { get; init; }
src/NotifyHub/NotificationMessage.cs:33
- The
Silentdocs say it's not applicable to Webhook, butWebhookChannelincludessilentin the generic payload. Align the XML docs with the implementation (and README) to avoid misleading API consumers.
/// <summary>When true, sends a silent/background notification instead of a visible one:
/// APNs <c>content-available: 1</c> (no <c>alert</c>/<c>sound</c>), FCM a data-only message
/// (no <c>notification</c> key - <see cref="Data"/> only), WebPush a
/// <c>Notification(..., { silent: true })</c> hint for the host's own service worker.
/// Useful for background sync. Default false (a normal, visible notification). Not
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+48
to
+55
| if (maxConcurrency is null) | ||
| { | ||
| var tasks = subscriptions.Select(subscription => SendOneAsync(subscription, message, channels, ct)); | ||
| return await Task.WhenAll(tasks); | ||
| } | ||
|
|
||
| using var throttle = new SemaphoreSlim(maxConcurrency.Value); | ||
| var throttledTasks = subscriptions.Select(async subscription => |
Comment on lines
+120
to
132
| var message = new NotificationMessage | ||
| { | ||
| Title = req.Title, | ||
| Body = req.Body, | ||
| Url = req.Url, | ||
| Data = req.Data, | ||
| Badge = req.Badge, | ||
| Sound = req.Sound, | ||
| Silent = req.Silent, | ||
| ImageUrl = req.ImageUrl, | ||
| }; | ||
| var results = await sender.SendAsync(message, targets.Select(t => t.Subscription), req.Channels, req.MaxConcurrency); | ||
|
|
Comment on lines
+58
to
62
| var messageFields = new Dictionary<string, object?> | ||
| { | ||
| message = new | ||
| { | ||
| token = subscription.DeviceToken, | ||
| notification = new { title = message.Title, body = message.Body }, | ||
| data = message.Data, | ||
| }, | ||
| ["token"] = subscription.DeviceToken, | ||
| ["data"] = message.Data, | ||
| }; |
Comment on lines
+65
to
+73
| var payload = new Dictionary<string, object>{ ["aps"] = aps }; | ||
| if (message.Data is not null) | ||
| { | ||
| // Apple convention: custom data lives as top-level keys alongside "aps", not nested. | ||
| foreach (var (key, value) in message.Data) | ||
| payload[key] = value; | ||
| } | ||
| if (message.Url is not null) | ||
| payload["url"] = message.Url; |
Comment on lines
+20
to
+22
| /// <summary>App icon badge count. Maps to APNs <c>aps.badge</c>. Not applicable to | ||
| /// WebPush/FCM/Webhook/Email - ignored there. Leave unset to not touch the app's existing | ||
| /// badge count (Apple's default behavior when this field is omitted).</summary> |
|
🎉 This PR is included in version 0.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Fix APNs dropping Data/Url; add Badge/Sound/Silent/ImageUrl;