feat(push): iOS Live Activity support via APNs broadcast channels#2253
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe PR adds APNs broadcast channel support to the push admin API, with new type declarations, runtime request methods, and tests for creating broadcast channels and managing Live Activity start, update, and end flows. ChangesAPNs Broadcast Live Activity Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
temporarily deployed
to
staging/pull/2253/bundle-report
![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
🤖 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 `@ably.d.ts`:
- Line 3710: Remove the blank lines between JSDoc tags in the affected
declarations so the docblocks no longer trigger the “Expected no lines between
tags” lint error. Update the JSDoc blocks around the referenced declaration
group in ably.d.ts, and apply the same cleanup consistently to the other
affected blocks mentioned in the comment.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 269deb54-60c8-48e3-8653-f7fe79b347fe
📒 Files selected for processing (2)
ably.d.tssrc/common/lib/client/push.ts
ttypic
force-pushed
the
AIT-794/apns-channel-implementation
branch
from
da0431f to
0a4779d
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2253/bundle-report
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)
src/common/lib/client/push.ts (2)
117-144: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPropagate
pushFullWaitto APNs broadcast/live-activity requestscreateApnsBroadcastandLiveActivity._postshould addfullWait: 'true'toparamsso these endpoints behave likepublishand the other push-mutating calls; otherwisepushFullWaitis ignored here.🤖 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/common/lib/client/push.ts` around lines 117 - 144, The APNs broadcast and live-activity push paths are missing the full-wait request flag, so `pushFullWait` is ignored in these mutating calls. Update `createApnsBroadcast` and `LiveActivity._post` in the push client code to include `fullWait: 'true'` in their request `params`, matching the behavior used by `publish` and the other push mutation methods.
302-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore
fullWaitforwarding inDeviceRegistrations.remove. This is the only push-delete path that dropsclient.options.pushFullWait, so callers lose the synchronous-delete behavior other admin methods still provide.🤖 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/common/lib/client/push.ts` around lines 302 - 326, Restore forwarding of client.options.pushFullWait in DeviceRegistrations.remove so the push delete call matches the other admin delete paths. Update the remove method in DeviceRegistrations to pass the fullWait flag through the Resource['delete'] invocation, using the existing client.options.pushFullWait value alongside the current headers, params, and deviceId handling.
🧹 Nitpick comments (2)
src/common/lib/client/push.ts (2)
154-172: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo validation that
recipienthas a target.
startdoesn't validate that at least one ofrecipient.channelsorrecipient.deviceIdis provided; if a caller supplies neither, the request silently posts with nochannels/deviceIdfield, likely producing an unhelpful server-side error rather than a clear client-side one.🤖 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/common/lib/client/push.ts` around lines 154 - 172, The start method in push client does not validate that recipient has a delivery target before calling _post. Add an explicit check in start(params) to require at least one of recipient.channels or recipient.deviceId, and fail fast with a clear client-side error if neither is present. Keep the validation close to the existing recipient destructuring/body construction so the behavior is easy to find and maintain.
117-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline request/response types duplicate the public contracts.
createApnsBroadcast's options/return shape andLiveActivity.start/update/endparameter shapes are hand-written inline object types here, while the PR objectives state named types (PushApnsBroadcastOptions,PushApnsBroadcast,PushLiveActivitystart/update/end params) are added toably.d.ts. Duplicating the shape in two places risks type drift as the public API evolves (e.g., an added optional field inably.d.tswouldn't be reflected here without a corresponding edit).Consider importing and using the canonical types from the public contracts module instead of re-declaring them inline.
Also applies to: 154-190
🤖 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/common/lib/client/push.ts` around lines 117 - 144, Inline request/response types in createApnsBroadcast and LiveActivity.start/update/end are duplicating the public API contracts and can drift from the named types added to ably.d.ts. Update these methods to use the canonical exported types (PushApnsBroadcastOptions, PushApnsBroadcast, and PushLiveActivity start/update/end param types) instead of re-declaring object shapes inline. Keep the method logic unchanged; only replace the local type annotations so the client stays aligned with the public contract definitions.
🤖 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 `@src/common/lib/client/push.ts`:
- Around line 117-144: The APNs broadcast and live-activity push paths are
missing the full-wait request flag, so `pushFullWait` is ignored in these
mutating calls. Update `createApnsBroadcast` and `LiveActivity._post` in the
push client code to include `fullWait: 'true'` in their request `params`,
matching the behavior used by `publish` and the other push mutation methods.
- Around line 302-326: Restore forwarding of client.options.pushFullWait in
DeviceRegistrations.remove so the push delete call matches the other admin
delete paths. Update the remove method in DeviceRegistrations to pass the
fullWait flag through the Resource['delete'] invocation, using the existing
client.options.pushFullWait value alongside the current headers, params, and
deviceId handling.
---
Nitpick comments:
In `@src/common/lib/client/push.ts`:
- Around line 154-172: The start method in push client does not validate that
recipient has a delivery target before calling _post. Add an explicit check in
start(params) to require at least one of recipient.channels or
recipient.deviceId, and fail fast with a clear client-side error if neither is
present. Keep the validation close to the existing recipient destructuring/body
construction so the behavior is easy to find and maintain.
- Around line 117-144: Inline request/response types in createApnsBroadcast and
LiveActivity.start/update/end are duplicating the public API contracts and can
drift from the named types added to ably.d.ts. Update these methods to use the
canonical exported types (PushApnsBroadcastOptions, PushApnsBroadcast, and
PushLiveActivity start/update/end param types) instead of re-declaring object
shapes inline. Keep the method logic unchanged; only replace the local type
annotations so the client stays aligned with the public contract definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 992de5b7-2bea-429b-943f-d0e318328f2d
📒 Files selected for processing (4)
ably.d.tssrc/common/lib/client/push.tstest/uts/rest/unit/push/push_admin_apns_broadcast.test.tstest/uts/rest/unit/push/push_admin_live_activity.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ably.d.ts
| /** | ||
| * The targeted recipients of the push-to-start notification. | ||
| */ | ||
| recipient: { |
There was a problem hiding this comment.
RSH1e1 describes this as
is either a list of
channelsor a singledeviceId
Can we improve the type by having a union between { channels } | { deviceId } so at least one field is required?
There was a problem hiding this comment.
and possible have an explicit check and throw an error in the .start() method?
and then a corresponding test for possible combinations
There was a problem hiding this comment.
added error, narrowed down type, but it's not for UTS, it won't matter for statically-typed languages
There was a problem hiding this comment.
but it's not for UTS, it won't matter for statically-typed languages
Huh, interesting. I see why we wouldn't want it to be in the UTS, but not including the tests in languages where it actually helps seems wrong
| /** | ||
| * Set to `1` to cache the last update payload so late-joining devices receive the current content state immediately on subscription. Set to `0` to disable caching. | ||
| */ | ||
| messageStoragePolicy: 0 | 1; |
There was a problem hiding this comment.
can we have an actual string enum for this in public API types? and then decode to the integer when sending to the server
There was a problem hiding this comment.
discussed earlier, when doing DR, don't remember why, but agreed to use numbers, I'll ask why, but let's not block on this.
There was a problem hiding this comment.
uhh, sure. A bit uncomfortable for me to approve as we don't have raw integers anywhere else in the codebase, it's always a string enum in public types.
ttypic
force-pushed
the
AIT-794/apns-channel-implementation
branch
from
0a4779d to
3c1eadf
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2253/bundle-report
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/common/lib/client/push.ts (2)
117-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse declared public types instead of inline shapes.
The public contracts layer already declares
PushApnsBroadcastOptions/PushApnsBroadcasttypes for this operation. This method redefines the option and return shapes inline ({ messageStoragePolicy: 0 | 1 },{ id: string; apnsChannelId: string }), so a future change to the public.d.tscontract won't be enforced by the compiler here, risking silent drift between the API surface and its implementation.♻️ Suggested type reuse
- async createApnsBroadcast(options: { messageStoragePolicy: 0 | 1 }): Promise<{ id: string; apnsChannelId: string }> { + async createApnsBroadcast(options: API.PushApnsBroadcastOptions): Promise<API.PushApnsBroadcast> {Separately,
publish()mixesclient.options.pushFullWaitintoparams(Line 111) but this method does not. Please confirm this omission is intentional for the broadcast-channel-creation endpoint (the prior discussion onfullWaitremoval was specifically about Live Activity endpoints, notcreateApnsBroadcast).🤖 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/common/lib/client/push.ts` around lines 117 - 144, The createApnsBroadcast method in PushClient is using inline request/response shapes instead of the જાહેર contract types, so update its signature and related locals to reuse PushApnsBroadcastOptions and PushApnsBroadcast from the public types layer. Also review the params setup in createApnsBroadcast to confirm whether pushFullWait should be omitted or explicitly added for this endpoint, since publish() handles it differently and the omission may need to match the intended API contract.
154-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated request-body construction and reuse declared param types.
start,update, andendeach redeclare the same inline{ apnsBroadcast, apns, headers? }shape and repeat theif (headers) body.headers = headerspattern. Extracting a small shared helper would reduce duplication as more Live Activity actions are added. Additionally, per the PR's public contracts layer, dedicated parameter types (PushLiveActivityStart..., etc.) already exist — using them here instead of inline object types would keep the implementation and the.d.tscontract in sync.♻️ Suggested consolidation
+ private buildActivityBody(apns: any, headers?: Record<string, string>): Record<string, any> { + const body: Record<string, any> = { apns }; + if (headers) { + body.headers = headers; + } + return body; + } + async update(params: { apnsBroadcast: string; apns: any; headers?: Record<string, string> }): Promise<void> { const { apnsBroadcast, apns, headers } = params; - const body: Record<string, any> = { apns }; - if (headers) { - body.headers = headers; - } + const body = this.buildActivityBody(apns, headers); await this._post(apnsBroadcast, 'broadcast', body); } async end(params: { apnsBroadcast: string; apns: any; headers?: Record<string, string> }): Promise<void> { const { apnsBroadcast, apns, headers } = params; - const body: Record<string, any> = { apns }; - if (headers) { - body.headers = headers; - } + const body = this.buildActivityBody(apns, headers); await this._post(apnsBroadcast, 'end', body); }Also worth confirming: does
end()genuinely require anapnspayload, or could it be a bodiless/minimal request in some cases? The provided test snippets only coverstart/update; please verify against the Live Activity test file forend.🤖 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/common/lib/client/push.ts` around lines 154 - 201, The Live Activity methods in PushClient repeat the same request-body assembly and use inline parameter shapes, so consolidate the shared body/header construction into a small helper and switch start, update, and end to the existing dedicated parameter types instead of redeclaring `{ apnsBroadcast, apns, headers? }` inline. Keep the public contract aligned by reusing the declared types already available for the Live Activity actions, and verify whether end in PushClient really needs an apns payload or should send a minimal/bodiless request based on the Live Activity tests.
🤖 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 `@src/common/lib/client/push.ts`:
- Around line 117-144: The createApnsBroadcast method in PushClient is using
inline request/response shapes instead of the જાહેર contract types, so update
its signature and related locals to reuse PushApnsBroadcastOptions and
PushApnsBroadcast from the public types layer. Also review the params setup in
createApnsBroadcast to confirm whether pushFullWait should be omitted or
explicitly added for this endpoint, since publish() handles it differently and
the omission may need to match the intended API contract.
- Around line 154-201: The Live Activity methods in PushClient repeat the same
request-body assembly and use inline parameter shapes, so consolidate the shared
body/header construction into a small helper and switch start, update, and end
to the existing dedicated parameter types instead of redeclaring `{
apnsBroadcast, apns, headers? }` inline. Keep the public contract aligned by
reusing the declared types already available for the Live Activity actions, and
verify whether end in PushClient really needs an apns payload or should send a
minimal/bodiless request based on the Live Activity tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97e68df2-bc7b-4317-b6b5-e3a7dfda5658
📒 Files selected for processing (4)
ably.d.tssrc/common/lib/client/push.tstest/uts/rest/unit/push/push_admin_apns_broadcast.test.tstest/uts/rest/unit/push/push_admin_live_activity.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/uts/rest/unit/push/push_admin_apns_broadcast.test.ts
- test/uts/rest/unit/push/push_admin_live_activity.test.ts
- ably.d.ts
| /** | ||
| * Restrict the push-to-start notification to a single device. Provide either `channels` or `deviceId`. | ||
| */ | ||
| deviceId?: string; |
There was a problem hiding this comment.
can we make channels and deviceId not optional here?
There was a problem hiding this comment.
Approving, two small oustanding points:
- make non optional
channelsanddeviceId - As discussed on standup,
messageStoragePolicyis Apple's related field, so keeping it as an integer would be fine as this is what Apple uses. We agreed to at least include the link to the Apple's docs for this. Potentially we may decide to change to the enum if the team agrees it will be of benefit.
github-actions
Bot
temporarily deployed
to
staging/pull/2253/bundle-report
- Introduced `createApnsBroadcast` function in the Push client to support APNs broadcast channels. - Added support for defining `messageStoragePolicy` options. - Updated `ably.d.ts` to include types for `PushApnsBroadcastOptions` and `PushApnsBroadcast`.
- Introduced `LiveActivity` class to manage the lifecycle of iOS Live Activities (start, update, end) via APNs broadcast channels. - Updated `ably.d.ts` with types for `PushLiveActivity` and associated parameters.
- Updated `PushChannelOptions` to set `channels` and `deviceId` as mandatory fields. - Added a reference link for `messageStoragePolicy` in `PushApnsBroadcastOptions`.
ttypic
force-pushed
the
AIT-794/apns-channel-implementation
branch
from
a1691fb to
eaf29d6
Compare
2 participants
Spec: ably/specification#500
Summary
Adds Push Admin support for managing iOS Live Activities through APNs broadcast channels. This introduces a two-part API: creating a broadcast channel, then driving the Live Activity lifecycle (start → update → end) over it.
Creates an APNs broadcast channel for an iOS Live Activity. Call once before starting the activity and persist the returned ids for the session.
Admin.createApnsBroadcastmethod (POST /push/apnsBroadcastChannels).PushApnsBroadcastOptions,PushApnsBroadcast.push.admin.liveActivitylifecycle APIA new LiveActivity class exposed as push.admin.liveActivity, managing the lifecycle of a Live Activity over a broadcast channel:
POST /push/apnsBroadcastChannels/{id}/start(supports targeting all subscribed channels, optionally restricted to a single deviceId).POST /push/apnsBroadcastChannels/{id}/broadcast(optional apns-priority / apns-expiration headers).POST /push/apnsBroadcastChannels/{id}/end; the broadcast id is no longer valid afterwards.PushLiveActivity,PushLiveActivityStartParams,PushLiveActivityUpdateParams,PushLiveActivityEndParams.Summary by CodeRabbit
Summary by CodeRabbit