Add APNs VoIP send path for Transport=voip notifications#158
Conversation
When core sends a push tagged apple_voip_rn{,beta}, dispatch it through
the same Apple key as the standard target but emit a VoIP-shaped APNs
request: apns-push-type: voip, priority 10, topic <bundle>.voip, flat
custom keys (channel_id, server_id, post_id, thread_id,
sender_id, id_loaded, ack_id, signature; plus sender_name and
channel_name when populated).
The mobile client uses this to distinguish flavors of a cancel push: "answered_elsewhere" maps to CXCallEndedReason.answeredElsewhere (silent dismiss when the user answers on another device); anything else stays at .unanswered (the caller-hung-up cancel-ring case).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds iOS VoIP push support: routes on PushNotification.Transport=="voip", builds VoIP APNs requests (PushTypeVOIP, .voip topic, content-available and routing fields), centralizes APNs dispatch, expands metrics with a transport label, redacts tokens in logs, adds tests, README, and a go.mod indirect dependency. ChangesiOS VoIP Push Notifications
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (1)
7-7: ⚡ Quick winDocument
Categoryforwarding in the VoIP payload contract.Please add a short note that
Categoryis forwarded for VoIP pushes (and omitted when empty), since clients rely on it to distinguish cancel reasons.🤖 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 `@README.md` at line 7, Add a short note to the VoIP delivery paragraph clarifying that the APNs `Category` field from the incoming notification is forwarded into the VoIP payload for platforms `apple_voip_rn` and `apple_voip_rnbeta` (which map to `ApplePushSettings`), and that the proxy omits the `Category` when it is empty; this ensures clients can rely on `Category` to distinguish cancel reasons in VoIP pushes.server/apple_notification_test.go (1)
16-35: ⚡ Quick winAlias test is validating duplicated logic, not the real dispatch alias path.
TestVoIPPlatformDispatchAliasre-implements the mapping inline, so a regression in the production remap path can still pass this test. Please route this assertion through the production aliasing function/path (or extract a shared helper used by both code and test).🤖 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 `@server/apple_notification_test.go` around lines 16 - 35, TestVoIPPlatformDispatchAlias is re-implementing the VoIP->non-VoIP mapping instead of exercising the production remap path; change the test to call the real aliasing function (or a shared helper) used by runtime dispatch so it validates actual behavior. Locate the production remap/alias function (the function that uses applePlatformVoIPPrefix to produce the final platform string) and replace the inline logic in TestVoIPPlatformDispatchAlias with a call to that function (or extract and import the shared helper used by both production code and tests), then assert the returned value equals tc.expected.
🤖 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.
Nitpick comments:
In `@README.md`:
- Line 7: Add a short note to the VoIP delivery paragraph clarifying that the
APNs `Category` field from the incoming notification is forwarded into the VoIP
payload for platforms `apple_voip_rn` and `apple_voip_rnbeta` (which map to
`ApplePushSettings`), and that the proxy omits the `Category` when it is empty;
this ensures clients can rely on `Category` to distinguish cancel reasons in
VoIP pushes.
In `@server/apple_notification_test.go`:
- Around line 16-35: TestVoIPPlatformDispatchAlias is re-implementing the
VoIP->non-VoIP mapping instead of exercising the production remap path; change
the test to call the real aliasing function (or a shared helper) used by runtime
dispatch so it validates actual behavior. Locate the production remap/alias
function (the function that uses applePlatformVoIPPrefix to produce the final
platform string) and replace the inline logic in TestVoIPPlatformDispatchAlias
with a call to that function (or extract and import the shared helper used by
both production code and tests), then assert the returned value equals
tc.expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2dc483e-d03d-4575-901b-ca3e9c873884
📒 Files selected for processing (5)
README.mdserver/apple_notification_server.goserver/apple_notification_test.goserver/push_notification.goserver/server.go
lieut-data
left a comment
lieut-data
left a comment
There was a problem hiding this comment.
I'm new to this code as well as push notifications in general, but a few thoughts to help get the ball moving on the suite of PRs.
|
|
||
| ## VoIP push notifications (iOS Calls) | ||
|
|
||
| The proxy delivers PushKit / VoIP pushes for iOS calls under the platform identifiers `apple_voip_rn` and `apple_voip_rnbeta`. These are aliased internally to the existing `apple_rn` / `apple_rnbeta` `ApplePushSettings` entries — no extra configuration block is required; the same APNs key is reused. The proxy emits a VoIP-shaped APNs request (`apns-push-type: voip`, topic `<ApplePushTopic>.voip`, minimal payload) when an incoming notification's platform carries the `apple_voip_` prefix. |
There was a problem hiding this comment.
I'm stumbling over the infix parsing and string juggling here and in the other PRs: could you elaborate on the current design?
Should we instead model the desired APNS push type explicitly in the payload? The configuration stays the same, the target platform remains unchanged, and the push proxy continues to vary the push type on demand. And I suppose this would allow a future use of the background push type if we ever went in that direction. (If APNSPushType is too granular, even just a VoIP flag signalling intent at the payload level seems simpler, at least on the surface.)
Secondly, are apple_rn and apple_rnbeta names universal targets, or just current conventions from our hosted push proxy? I think the code is agnostic, but might be good to clarify the documentation here if we're not actually bound to these platform identifiers.
There was a problem hiding this comment.
The idea was to avoid a duplicate configuration just because of the voip vs normal push notification, the initial design decided based on the "Platform" and that was either apple or android with variants for "rn" and "rnbeta".
I'm with you when it comes to the model, in fact that was my initial direction, I ended up going this way cause modifying the model meant to introduce a breaking change in the plugin as we dropped support for MySQL and the plugin still has that code and I did not want to make that decision nor delay this.
I even had to add a very hacky approach for a type of clear notification, that I think it should be avoided.
A lot of the decisions were made based on the above.
There was a problem hiding this comment.
as we dropped support for MySQL and the plugin still has that code and I did not want to make that decision nor delay this.
To clarify, is the existing MySQL support in the Calls plugin a blocker? We can absolutely drop that as a consideration, and I'm happy to help with that.
I'm with you when it comes to the model, in fact that was my initial direction, I ended up going this way cause modifying the model meant to introduce a breaking change in the plugin
I'd love to dig into this and enable your original design. Let me find time to 1:1 and we can unblock this ASAP.
Thanks, @enahum!
| if me.metrics != nil { | ||
| me.metrics.incrementNotificationTotal(PushNotifyApple, msg.Type) | ||
| } |
There was a problem hiding this comment.
Do we have metrics for this new VoIP path? (Do we need to key off SubType somewhere?)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/apple_notification_test.go (1)
83-161: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRe-add VoIP
categorypayload assertions to protect the Calls contract.Given Calls routing depends on
category, this suite should assert it is forwarded when non-empty and omitted when empty. That would catch the current payload regression early.🤖 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 `@server/apple_notification_test.go` around lines 83 - 161, The test suite is missing assertions for the VoIP "category" field; update the tests around buildVoIPNotification and PushNotification to assert that when msg.Category is set it appears in the marshalled payload (body["category"]) and when msg.Category is empty it is omitted (no key present), mirroring the existing patterns for sender_name/channel_name and ack_id; modify the first test to assert the populated category is forwarded and add/adjust the IdLoaded-mode test (and the missing-ack test pattern) to assert the category key is absent when empty.server/apple_notification_server.go (1)
325-346:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward
Categoryin VoIP payload to preserve call-end semantics.
buildVoIPNotificationcurrently dropsmsg.Category. That breaks the Calls contract forType=clear, SubType=callswhere the client distinguishesanswered_elsewherevs other end reasons. Please includecategorywhen non-empty.Proposed fix
func (me *AppleNotificationServer) buildVoIPNotification(msg *PushNotification) *apns.Notification { data := payload.NewPayload(). ContentAvailable(). Custom("type", msg.Type). Custom("sub_type", msg.SubType). Custom("channel_id", msg.ChannelID). Custom("server_id", msg.ServerID). Custom("post_id", msg.PostID). Custom("thread_id", msg.RootID). Custom("sender_id", msg.SenderID). Custom("id_loaded", msg.IsIDLoaded) + + if msg.Category != "" { + data.Custom("category", msg.Category) + }🤖 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 `@server/apple_notification_server.go` around lines 325 - 346, In buildVoIPNotification, include msg.Category in the VoIP payload when non-empty so call-end semantics are preserved; modify the payload construction in AppleNotificationServer.buildVoIPNotification to add data.Custom("category", msg.Category) conditional on msg.Category != "" (similar to existing sender_name/channel_name checks) so the client can distinguish answered_elsewhere vs other end reasons for Type=clear, SubType=calls.
🤖 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.
Outside diff comments:
In `@server/apple_notification_server.go`:
- Around line 325-346: In buildVoIPNotification, include msg.Category in the
VoIP payload when non-empty so call-end semantics are preserved; modify the
payload construction in AppleNotificationServer.buildVoIPNotification to add
data.Custom("category", msg.Category) conditional on msg.Category != "" (similar
to existing sender_name/channel_name checks) so the client can distinguish
answered_elsewhere vs other end reasons for Type=clear, SubType=calls.
In `@server/apple_notification_test.go`:
- Around line 83-161: The test suite is missing assertions for the VoIP
"category" field; update the tests around buildVoIPNotification and
PushNotification to assert that when msg.Category is set it appears in the
marshalled payload (body["category"]) and when msg.Category is empty it is
omitted (no key present), mirroring the existing patterns for
sender_name/channel_name and ack_id; modify the first test to assert the
populated category is forwarded and add/adjust the IdLoaded-mode test (and the
missing-ack test pattern) to assert the category key is absent when empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 832d70c2-c2fe-42ef-80cf-1c5d329ed8b3
📒 Files selected for processing (6)
server/android_notification_server.goserver/apple_notification_server.goserver/apple_notification_test.goserver/push_notification.goserver/push_notification_test.goserver/server.go
✅ Files skipped from review due to trivial changes (1)
- server/push_notification_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/server.go
- server/android_notification_server.go
enahum
changed the title
lieut-data
left a comment
lieut-data
left a comment
There was a problem hiding this comment.
Thanks for tweaking the data model, @enahum! A handful of follow up comments, but not strictly blocking at this point.
|
|
||
| // PushTransportVoIP is the value of PushNotification.Transport that | ||
| // dispatches via the PushKit/VoIP send path. | ||
| PushTransportVoIP = "voip" |
There was a problem hiding this comment.
Could we add a PushTransportDefault = "" (or PushTransportNone?) to clarify the meaning of the various "" added to the code?
There was a problem hiding this comment.
Sure I think that will make it more readable
|
|
||
| // RedactToken returns the first 8 chars of a device token followed by an | ||
| // ellipsis, for safe inclusion in logs. | ||
| func RedactToken(token string) string { |
There was a problem hiding this comment.
nit: do we need to export this from the server package, or can we keep it redactToken and internal?
There was a problem hiding this comment.
I'll check.. I believe it ahould be possible
| want string | ||
| }{ | ||
| {"empty", "", ""}, | ||
| {"short token passes through", "1234", "1234"}, |
There was a problem hiding this comment.
I don't know anything about the tokens in these cases, but wondering if there's a more holistic approach to token handling we should be considering? (i.e. not logging it at all, or if we do, logging a one-way hash)
There was a problem hiding this comment.
Well is the tokens are long enough, but we definitely need a way to identify them and match with the session in the db in case we need to debug something, so a one way hash will make it very difficult imo
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/android_notification_server.go (1)
221-232:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent transport label: use
PushTransportDefaultinstead of"".These metric increments pass an empty transport string, while
incrementNotificationTotal(Line 163) andincrementRemoval(Line 199) on the same Android path passPushTransportDefault. This emits split label values (transport=""vstransport="default") for the same notification flow, which fragments counters and breaks aggregation/dashboards that filter by transport.🔧 Proposed fix to align the transport label
if me.metrics != nil { - me.metrics.incrementFailure(PushNotifyAndroid, pushType, "", reason) + me.metrics.incrementFailure(PushNotifyAndroid, pushType, PushTransportDefault, reason) } return NewErrorPushResponse(err.Error()) } if me.metrics != nil { if msg.AckID != "" { - me.metrics.incrementSuccessWithAck(PushNotifyAndroid, pushType, "") + me.metrics.incrementSuccessWithAck(PushNotifyAndroid, pushType, PushTransportDefault) } else { - me.metrics.incrementSuccess(PushNotifyAndroid, pushType, "") + me.metrics.incrementSuccess(PushNotifyAndroid, pushType, PushTransportDefault) } }🤖 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 `@server/android_notification_server.go` around lines 221 - 232, The metric calls on the Android notification path are using an empty transport label (""), causing split Prometheus labels; update the calls to use the canonical constant PushTransportDefault instead. Specifically, replace the third argument in me.metrics.incrementFailure(PushNotifyAndroid, pushType, "", reason) and in both me.metrics.incrementSuccessWithAck(PushNotifyAndroid, pushType, "") and me.metrics.incrementSuccess(PushNotifyAndroid, pushType, "") with PushTransportDefault so they match other calls like incrementNotificationTotal and incrementRemoval.
🤖 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.
Outside diff comments:
In `@server/android_notification_server.go`:
- Around line 221-232: The metric calls on the Android notification path are
using an empty transport label (""), causing split Prometheus labels; update the
calls to use the canonical constant PushTransportDefault instead. Specifically,
replace the third argument in me.metrics.incrementFailure(PushNotifyAndroid,
pushType, "", reason) and in both
me.metrics.incrementSuccessWithAck(PushNotifyAndroid, pushType, "") and
me.metrics.incrementSuccess(PushNotifyAndroid, pushType, "") with
PushTransportDefault so they match other calls like incrementNotificationTotal
and incrementRemoval.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5bd429d-cbb7-423e-b868-0723c14077c7
📒 Files selected for processing (6)
server/android_notification_server.goserver/apple_notification_server.goserver/apple_notification_test.goserver/push_notification.goserver/push_notification_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/apple_notification_test.go
- server/apple_notification_server.go
3 participants
Summary
Push proxy side of iOS PushKit + CallKit ringing. Adds an APNs VoIP
send path for the new
apple_voip_rn{,beta}platforms that the serveremits when a Calls push has a VoIP token registered for the recipient.
Ticket Link
Relates-to: https://mattermost.atlassian.net/browse/MM-68727