feat: Add apns-id header to client return value#195
feat: Add apns-id header to client return value#195mtrezza merged 3 commits intoparse-community:masterfrom
apns-id header to client return value#195Conversation
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughClient now captures Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![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
🤖 Fix all issues with AI agents
In @lib/client.js:
- Around line 519-521: The statement setting header['apns-id'] when
notificationId is truthy is missing a terminating semicolon; update the
conditional block (the code that checks notificationId and assigns
header['apns-id'] = notificationId) to end the assignment line with a semicolon
to fix the syntax error.
🧹 Nitpick comments (1)
lib/client.js (1)
569-569: Use consistent quote style.This line uses double quotes while the surrounding code (lines 566-568) uses single quotes for consistency.
♻️ Proposed fix
- notificationId = headers["apns-id"]; + notificationId = headers['apns-id'];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/client.js
🧰 Additional context used
🧬 Code graph analysis (1)
lib/client.js (1)
lib/notification/index.js (1)
headers(74-74)
🔇 Additional comments (2)
lib/client.js (2)
538-538: LGTM!The variable declaration follows the same pattern as other response header variables.
586-586: LGTM!The
notificationIdis correctly passed tocreateHeaderObject, matching the updated method signature and following the same pattern as other response headers.
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
12fc01c to
c14ef4e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 95.84% 95.86% +0.01%
==========================================
Files 23 23
Lines 842 846 +4
==========================================
+ Hits 807 811 +4
Misses 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/client.js (1)
1616-1669: Consider addingapns-idtest coverage for ManageChannelsClient.The device and broadcasts tests now validate
apns-idpropagation, but the ManageChannelsClient tests don't include this coverage. Since the underlyingrequest()method is shared, the feature should work, but adding a test for channels (similar to the device test pattern) would provide additional confidence.This is optional since the core logic is already tested via the device tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/client.jstest/client.js
🧰 Additional context used
🧬 Code graph analysis (2)
lib/client.js (1)
test/client.js (1)
notificationId(251-251)
test/client.js (1)
lib/client.js (1)
notificationId(538-538)
🔇 Additional comments (7)
lib/client.js (4)
503-523: LGTM! Clean extension to exposeapns-idin return values.The function signature extension is backwards-compatible since the new
notificationIdparameter is optional and guarded by the truthiness check before adding to the header object.
538-538: LGTM!Variable properly initialized alongside other response header variables.
569-569: LGTM!Correctly extracts
apns-idfrom response headers, following the same pattern as other APNs headers (apns-unique-id,apns-request-id,apns-channel-id).
586-591: LGTM!The
notificationIdis correctly passed tocreateHeaderObject, ensuring theapns-idis included in all resolve/reject outcomes where header information is propagated.test/client.js (3)
137-138: LGTM! Test correctly validatesapns-idpropagation for device notifications.The test properly:
- Defines the expected
apns-idvalue- Configures the mock server to return it in response headers
- Asserts that the client result includes the
apns-idAlso applies to: 150-150, 168-168
194-195: LGTM! Broadcasts test mirrors the device test pattern forapns-id.Also applies to: 207-207, 225-225
251-252: LGTM! Load test verifiesapns-idworks correctly under concurrent requests.Good coverage for ensuring the header propagation works correctly when handling a batch of 2000 simultaneous notifications.
Also applies to: 264-264, 282-282
|
I added the header to existing tests, hope that is satisfactory. The other propagated headers are also tested indirectly but are more directly linked to a specific type of api call. In my understanding the If you'd want a separate test for this specific header I'll do that. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/client.js (4)
137-168: Test correctly verifiesapns-idpropagation.The test properly validates that the
apns-idheader from the response is included in the returned result. The use of spread syntax to mergeadditionalHeaderInfowith the result is correct.Optional suggestion: Enhance test clarity
Consider adding a comment explaining that this test verifies the
apns-idheader propagation, or update the test description to reflect this behavior explicitly. This would make the test's purpose clearer:it('Treats HTTP 200 responses as successful for device', async () => { + // This test also verifies that the apns-id header is propagated to the result let didRequest = false;
297-363: Consider clarifying whyapns-idis not included in this test.This test validates session ping behavior and correctly omits the
apns-idheader (since pings aren't notification deliveries). However, it would be helpful to explicitly document this to distinguish it from the other tests that do includeapns-id.Optional: Add clarifying comment
it('Log pings for session', async () => { + // Note: This test does not include apns-id as it's testing ping responses, not notification delivery let establishedConnections = 0;
1913-1979: Consistent with device ping test - consider adding clarification.Like the device session ping test (lines 297-363), this test correctly omits the
apns-idheader since it's testing ping functionality rather than notification delivery. Consider adding a clarifying comment similar to the suggestion for the device ping test.
137-138: Consider using varied notification IDs for clarity.All tests use the same hardcoded
notificationId:'7dc35f9f-58d4-40dd-8c08-38ab811f57df'. While this is acceptable for mock tests, using different UUIDs per test (or test category) could improve clarity and make it easier to trace specific test scenarios.Example: Use test-specific IDs
it('Treats HTTP 200 responses as successful for device', async () => { // ... - const notificationId = '7dc35f9f-58d4-40dd-8c08-38ab811f57df'; + const notificationId = 'device-200-test-notification-id';Or use a UUID generator to create unique IDs per test if preferred.
Also applies to: 194-195, 251-252, 1622-1623, 1682-1682, 1746-1746, 1807-1807, 1867-1867
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/client.js
🧰 Additional context used
🧬 Code graph analysis (1)
test/client.js (1)
lib/client.js (3)
notificationId(538-538)requestId(536-536)uniqueId(535-535)
🔇 Additional comments (4)
test/client.js (4)
1622-1653: LGTM - Consistent test pattern for channels endpoint.The test correctly validates
apns-idpropagation for the channels endpoint, following the same pattern as the device endpoint tests.
1682-1719: LGTM - Properly tests multiple response headers.The test correctly validates that
apns-idis propagated along with other response headers (apns-channel-id,apns-request-id,apns-unique-id) in HTTP 201 responses.
1746-1779: LGTM - Good coverage of DELETE operations.The test correctly validates that
apns-idis propagated in HTTP 204 responses for DELETE operations, providing comprehensive coverage across different HTTP methods.
803-887: No changes needed—the proxy test intentionally omitsapns-idto test backward compatibility.The proxy tests (both device and channels variants) intentionally do not include the
apns-idheader in the response, unlike the standard "Treats HTTP 200 responses as successful" tests. This provides important test coverage for how the client handles responses that lack theapns-idheader. The client gracefully handles this case by extracting the header only if present. If clarity is desired for future maintainers, consider adding an inline comment explaining this is intentional backward-compatibility testing.
|
@RedPhoenixQ Thanks, we generally ask for not modifying existing tests if not necessary and add a new, specific test with an identifier that describes what it tests. So could you please revert the changes (unless necessary for existing tests to pass) and add a distinct test like |
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
c1c65c2 to
0c0257d
Compare
mtrezza
left a comment
mtrezza
left a comment
There was a problem hiding this comment.
Why are there 2 almost identical tests?
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
0c0257d to
d6aa431
Compare
|
I misunderstood the purpose of the ManageChannelsClient tests and there indeed doesn't need to be a separate test there as is also uses the Client. I removed the duplicate |
There was a problem hiding this comment.
Pull request overview
This PR adds support for capturing and returning the apns-id header from Apple Push Notification service (APNs) responses. The apns-id is a canonical UUID that identifies a notification, useful for debugging and tracking notification delivery.
Changes:
- Extended the client to extract and return the
apns-idheader from APNs responses - Added a new parameter to
createHeaderObjectto include the notification ID in results - Added comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/client.js | Added notificationId parameter to createHeaderObject, extracted apns-id from response headers, and included it in the returned header object |
| test/client.js | Added test case "Returns APNs notification ID in responses" that verifies apns-id is correctly extracted from responses and included in results, including concurrent request handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@RedPhoenixQ could you rebase the PR and see the test in #195 (comment) for ways to simplify your suggested test. |
RedPhoenixQ
force-pushed
the
expose-apns-id
branch
from
ba1ed89 to
9ea743a
Compare
|
Thank you for taking the time. I've simplified the test as per the recommendation (amending the commit) and rebased on master |
# [7.1.0](7.0.1...7.1.0) (2026-01-31) ### Features * Add `apns-id` header to client return value ([#195](#195)) ([5567de1](5567de1))
|
🎉 This change has been released in version 7.1.0 |
3 participants
This exposes the
apns-idheader that is returned to identify a notification for debugging and tracking.Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.